Skip to content
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

Bug: ConfigurationPolicy message for enforce omits objects when multiple namespaces are specified #116

Conversation

yiraeChristineKim
Copy link
Contributor

ConfigurationPolicy message for enforce omits objects when multiple namespaces are specified

When set to inform, the full list of objects is shown. But when set to enforce (the first two messages displayed), only the last object message is shown. In multiple namespaces, the message should be merged.

Should be fixed like this example:
pod [pod1] in namespace test1 found; [pod1] in namespace test2 found as specified, therefore this Object template is compliant

Ref: https://issues.redhat.com/browse/ACM-2604

@yiraeChristineKim yiraeChristineKim force-pushed the ACM-2604-multi-ns-cherrypick branch 2 times, most recently from d7e709f to aed9a71 Compare April 4, 2023 14:50
test/e2e/case5_multi_test.go Outdated Show resolved Hide resolved
if statusUpdateNeeded {
parentStatusUpdateNeeded = true
}

if mergeMessageEnforce {
r.Recorder.Event(&plc, eventNormal, fmt.Sprintf(plcFmtStr, plc.GetName()),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct sending event? is this message will attach to policy history? curious

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this event sending is necessary? Does it work without this block? It's buried code, but (assuming I'm looking at this right) statusUpdateNeeded propagates down the following function calls, and sendComplianceEvent is the one that finally defines an Event and calls r.Create() to deliver it:

Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This will be a great update! I've left some comments/questions to consider.

if statusUpdateNeeded {
parentStatusUpdateNeeded = true
}

if mergeMessageEnforce {
r.Recorder.Event(&plc, eventNormal, fmt.Sprintf(plcFmtStr, plc.GetName()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this event sending is necessary? Does it work without this block? It's buried code, but (assuming I'm looking at this right) statusUpdateNeeded propagates down the following function calls, and sendComplianceEvent is the one that finally defines an Event and calls r.Create() to deliver it:

controllers/configurationpolicy_controller.go Outdated Show resolved Hide resolved
controllers/configurationpolicy_controller.go Show resolved Hide resolved
ConfigurationPolicy message for enforce omits objects when multiple namespaces are specified

When set to inform, the full list of objects is shown. But when set to enforce (the first two messages displayed), only the last object message is shown. In multiple namespaces, the message should be merged.

Should be fixed like this example:
pod [pod1] in namespace test1 found; [pod1] in namespace test2 found as specified, therefore this Object template is compliant

Ref: https://issues.redhat.com/browse/ACM-2604

Signed-off-by: Yi Rae Kim <yikim@redhat.com>
Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold if others would like another review

LGTM! The existing code that used name == nil as a signal that it had been "handled" is making my head spin, but you solution appears to work alongside it well. Thanks for the update!

@openshift-ci
Copy link

openshift-ci bot commented Apr 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, JustinKuli, yiraeChristineKim

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:
  • OWNERS [JustinKuli,dhaiducek,yiraeChristineKim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yiraeChristineKim
Copy link
Contributor Author

/unhold

@yiraeChristineKim
Copy link
Contributor Author

/hold

@yiraeChristineKim
Copy link
Contributor Author

/unhold

@yiraeChristineKim yiraeChristineKim merged commit 48f6ec2 into open-cluster-management-io:main Apr 25, 2023
@yiraeChristineKim yiraeChristineKim deleted the ACM-2604-multi-ns-cherrypick branch February 29, 2024 20:06
dhaiducek pushed a commit to dhaiducek/config-policy-controller-1 that referenced this pull request Aug 26, 2024
…ement-io#116)

ConfigurationPolicy message for enforce omits objects when multiple namespaces are specified

When set to inform, the full list of objects is shown. But when set to enforce (the first two messages displayed), only the last object message is shown. In multiple namespaces, the message should be merged.

Should be fixed like this example:
pod [pod1] in namespace test1 found; [pod1] in namespace test2 found as specified, therefore this Object template is compliant

Ref: https://issues.redhat.com/browse/ACM-2604

Signed-off-by: Yi Rae Kim <yikim@redhat.com>
(cherry picked from commit 48f6ec2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants