Add a message to Trust checks if not a trust agent/controller#367
Merged
rcritten merged 1 commit intofreeipa:masterfrom Sep 15, 2025
Merged
Add a message to Trust checks if not a trust agent/controller#367rcritten merged 1 commit intofreeipa:masterfrom
rcritten merged 1 commit intofreeipa:masterfrom
Conversation
Previously it would return an empty SUCCESS message because if the server is not an agent and/or controller role then there is nothing to do. So return a message that indicates this. Note: this is a change in behavior. The test used to be skipped altogether so returned no result at all. Returning a result will help ensure that all tests were executed. Fixes: freeipa#85 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
There was a problem hiding this comment.
Hey @rcritten - I've reviewed your changes - here's some feedback:
- Consider extracting the repeated skip logic (yielding Result(self, SUCCESS, …) and return) into a shared helper or base method so you don’t have to copy it into every check class.
- Define the skip messages (e.g. “Skipped. Not a trust agent” / “Skipped. Not a trust controller”) as constants or via a helper function to guarantee consistency and simplify future updates.
- Double-check message formatting (capitalization, punctuation) against project style guidelines to ensure the new SUCCESS messages are uniform across all checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the repeated skip logic (yielding Result(self, SUCCESS, …) and return) into a shared helper or base method so you don’t have to copy it into every check class.
- Define the skip messages (e.g. “Skipped. Not a trust agent” / “Skipped. Not a trust controller”) as constants or via a helper function to guarantee consistency and simplify future updates.
- Double-check message formatting (capitalization, punctuation) against project style guidelines to ensure the new SUCCESS messages are uniform across all checks.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
frasertweedale
approved these changes
Sep 10, 2025
Collaborator
frasertweedale
left a comment
There was a problem hiding this comment.
Reviewed and tested. ACK.
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 6, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 6, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 6, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 6, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 7, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 7, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 7, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 17, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 18, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
ssidhaye
added a commit
to ssidhaye/freeipa
that referenced
this pull request
Nov 18, 2025
Previously the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com>
abbra
pushed a commit
to freeipa/freeipa
that referenced
this pull request
Nov 20, 2025
…the check would return an empty SUCCESS message. This is an update to the integration test since this involves a beahviour change Related PR: freeipa/freeipa-healthcheck#367 Related JIRA: https://issues.redhat.com/browse/RHEL-112752 Signed-off-by: Sumedh Sidhaye <ssidhaye@redhat.com> Reviewed-By: Florence Blanc-Renaud <frenaud@redhat.com> Reviewed-By: Rob Crittenden <rcritten@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously it would return an empty SUCCESS message because if the server is not an agent and/or controller role then there is nothing to do. So return a message that indicates this.
Note: this is a change in behavior. The test used to be skipped altogether so returned no result at all. Returning a result will help ensure that all tests were executed.
Fixes: #85
Summary by Sourcery
Return a success result with an explanatory message for all trust checks when the server is not a trust agent or controller, ensuring skipped tests still produce a result.
Enhancements:
Tests: