-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the conditions history in compliancyDetails #91
Fix the conditions history in compliancyDetails #91
Conversation
5c919b8
to
8544c74
Compare
8544c74
to
36e72c3
Compare
92572c7
to
2b8c969
Compare
The status.compliancyDetails[].conditions array was always limited to just a single condition due to a bug. This sets the limit to 10 instead. Relates: https://issues.redhat.com/browse/ACM-2708 Signed-off-by: mprahl <mprahl@users.noreply.github.com>
Signed-off-by: mprahl <mprahl@users.noreply.github.com>
2b8c969
to
3c98e22
Compare
I've been thinking about this, and I don't think it's a bug per se. This looks intentional since the status-sync is the thing that has the 10 history limit, so the Policy as the source of the history and the ConfigurationPolicy as only holding the current state. Though I could see the ConfigurationPolicy not having the same information as the Policy as a bug, but maybe we only need to address this in 2.7 as a result? Does the current behavior cause issues or is it just an inconsistency? |
@dhaiducek it doesn't cause issues with the status of the policy at all (besides the history inconsistency with config policies) but I would still say it's a bug since we have a conditions array that isn't really being used. If we want to only hold the latest condition in the config policy and have policies be responsible for history, that would be fine but I think we'd want the |
@dhaiducek the status-sync gets its history from status events sent by the ConfigurationPolicy, so the conditions array doesn't influence that. Having a history in the ConfigurationPolicy is still desirable as it contains more information than the status stored by the status-sync, in particular it shows information per object-template entry. |
Sorry about the noise on this PR @dhaiducek and @willkutler, but it's rebased on #90 and the tests are now passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanations/background! I think I was questioning whether a backport was necessary since the issue is targeted for 2.5 and 2.6, but it does seem like it'd be nice to have for debugging and consistency.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, mprahl, willkutler The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The status.compliancyDetails[].conditions array was always limited to just a single condition due to a bug. This sets the limit to 10 instead.
Relates:
https://issues.redhat.com/browse/ACM-2708