Add relatedObjs to unnamed musthave object #182

yiraeChristineKim · 2023-12-04T17:10:05Z

What the user wants:
When the object, even if no having name, turns compliant, appears in the list. We can see the object of Kind Node, compliant, and it does not match any specific name of the Kind object.

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

controllers/configurationpolicy_controller.go

test/e2e/case37_no_name_test.go

JustinKuli · 2023-12-04T20:04:37Z

test/e2e/case37_no_name_test.go

+	case37PolicyCSName        string = "case37-test-policy-clusterscope"
+)
+
+var _ = Describe("Test a namespace-scope policy that is missing name ", Ordered, func() {


These tests look great! I see tests for objects when it is compliant and when it is non-compliant (I was worried just looking through the code that it only covered one of those cases).

Do these changes also address similar cases for mustnothave policies?

if objShouldExist { . . . if objDetails.kind != "" && objDetails.name == "" {

It doesn't affect mustnothave. I thought mustnothave ok

I think when a mustnothave policy is Compliant, it would have a similar situation: its relatedObjects list would be empty. It doesn't look like it's necessary to address that for this issue, but it's something to consider in the future. I think it could be nice if there was always something in that list.

Can you give me example relatedObjects when it is mustnothave? To me.. it is mustnothave so it is right that it should not show anything

mprahl · 2023-12-11T21:43:56Z

controllers/configurationpolicy_controller.go

+
+					// Change reason to Resource found but does not match
+					if len(objNames) > 0 {
+						shouldAddCondensedRelatedObj = true


Why not just always set this to true instead of duplicating the list query performed in getNamesOfKind?

len(objNames) == 0 there is nothing to show on relatedobj. In other words, there is nothing to debug

I'd prefer if whenever the object template is not named and it's noncompliant, you just set shouldAddCondensedRelatedObj = true. Perhaps there are no objects, but that's okay. It makes the behavior consistent.

Also, I'm not sure if reasonWantFoundNoMatch is better than reasonWantFoundDNE, so if that's why you are duplicating the list query by calling getNamesOfKind again, then it's not worth it in my opinion.

um... to me, reasonWantFoundNoMatch is better. Because the name is * in relatedObj and users only see the reason on the console. reasonWantFoundNoMatch is for relatedObj.. isn't it? @JustinKuli how about yoiu

After this poll, we should decide mustnothave reason too

Sorry for slow response, if you're still looking for input, I think reasonWantFoundNoMatch makes more sense when the name is *. It doesn't make as much sense to say something "resource *, not found" when some instances of that resource do exist.

So for mustnothave, same kind of thing: if the names would be * or something similar, because some resources were found they just didn't fully match the policy criteria, then use a NoMatch kind of reason.

@JustinKuli Thanks!

controllers/configurationpolicy_controller.go

JustinKuli

/hold

Looks good to me! I'm not sure if Matt had any remaining requirements

controllers/configurationpolicy_utils.go

mprahl · 2024-01-05T13:32:01Z

test/e2e/case37_no_name_test.go

+				case37PolicyMustnothaveName, testNamespace, true, defaultTimeoutSeconds)
+			Expect(plc).ShouldNot(BeNil())
+		})
+		It("should have 0 Compliant relatedObject under the policy's status", func() {


I don't think this is correctly describing the test since it's expecting related objects to have a length of 1. Could you please reword it?

mprahl · 2024-01-05T13:32:13Z

test/e2e/case37_no_name_test.go

+				return utils.GetComplianceState(managedPlc)
+			}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
+
+			By("relatedObjects should not exist")


Suggested change

By("relatedObjects should not exist")

By("relatedObjects should exist")

mprahl · 2024-01-05T13:32:35Z

test/e2e/case37_no_name_test.go

+				// Getting relatedObjects takes longer time
+			}, defaultTimeoutSeconds*2, 1).Should(HaveLen(1))
+
+			By("Check the reasons of relatedObjects")


Suggested change

By("Check the reasons of relatedObjects")

By("Check the reason of the related object")

test/e2e/case37_no_name_test.go

mprahl · 2024-01-05T13:34:22Z

test/e2e/case37_no_name_test.go

+
+			By("Check the reason of relatedObjects")
+			reason := relatedObjects[0].(map[string]interface{})["reason"].(string)
+			Expect(reason).Should(Equal("Resource found but should not exist"))


Could you please also test that the name is not * here?

mprahl

This looks great! Just a few minor comments in the tests.

Ref: https://issues.redhat.com/browse/ACM-8782 Signed-off-by: Yi Rae Kim <yikim@redhat.com>

mprahl

Looks great!

/hold for @JustinKuli to take another look

JustinKuli

/unhold

openshift-ci · 2024-01-05T15:41:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl, 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,mprahl,yiraeChristineKim]

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

dhaiducek · 2024-01-05T16:11:13Z

__debug_bin2514188118

@yiraeChristineKim I think this file shouldn't have been merged. Can you revert this, please?

openshift-ci bot added do-not-merge/work-in-progress dco-signoff: yes approved labels Dec 4, 2023

yiraeChristineKim force-pushed the ACM-8782 branch from 4aa40f9 to 14c59de Compare December 4, 2023 18:44

yiraeChristineKim requested review from JustinKuli, dhaiducek and mprahl December 4, 2023 19:01

yiraeChristineKim marked this pull request as ready for review December 4, 2023 19:20

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 4, 2023

JustinKuli reviewed Dec 4, 2023

View reviewed changes

controllers/configurationpolicy_controller.go Outdated Show resolved Hide resolved

JustinKuli reviewed Dec 4, 2023

View reviewed changes

yiraeChristineKim force-pushed the ACM-8782 branch 4 times, most recently from 57af0a8 to 3eb1cd4 Compare December 8, 2023 22:34

yiraeChristineKim requested a review from JustinKuli December 11, 2023 11:17

mprahl reviewed Dec 11, 2023

View reviewed changes

controllers/configurationpolicy_controller.go Outdated Show resolved Hide resolved

mprahl reviewed Dec 11, 2023

View reviewed changes

controllers/configurationpolicy_controller.go Show resolved Hide resolved

yiraeChristineKim force-pushed the ACM-8782 branch from e82f43c to 8c2868d Compare December 14, 2023 16:18

yiraeChristineKim requested a review from mprahl December 14, 2023 16:50

yiraeChristineKim force-pushed the ACM-8782 branch 2 times, most recently from 01ab83e to 8d7b9ae Compare December 15, 2023 13:33

yiraeChristineKim commented Dec 15, 2023

View reviewed changes

controllers/configurationpolicy_controller.go Show resolved Hide resolved

yiraeChristineKim force-pushed the ACM-8782 branch 2 times, most recently from ebbd1c7 to 78d9c3f Compare January 2, 2024 18:15

JustinKuli previously approved these changes Jan 2, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Jan 2, 2024

openshift-ci bot assigned JustinKuli Jan 2, 2024

openshift-ci bot added the lgtm label Jan 2, 2024

mprahl reviewed Jan 2, 2024

View reviewed changes

controllers/configurationpolicy_utils.go Outdated Show resolved Hide resolved

yiraeChristineKim dismissed JustinKuli’s stale review via 410e9b9 January 3, 2024 01:41

yiraeChristineKim force-pushed the ACM-8782 branch from 78d9c3f to 410e9b9 Compare January 3, 2024 01:41

openshift-ci bot removed the lgtm label Jan 3, 2024

yiraeChristineKim force-pushed the ACM-8782 branch 4 times, most recently from b72dc03 to 615a10f Compare January 4, 2024 14:59

yiraeChristineKim requested review from mprahl and JustinKuli January 4, 2024 14:59

mprahl reviewed Jan 5, 2024

View reviewed changes

test/e2e/case37_no_name_test.go Show resolved Hide resolved

mprahl reviewed Jan 5, 2024

View reviewed changes

mprahl requested changes Jan 5, 2024

View reviewed changes

openshift-ci bot assigned mprahl Jan 5, 2024

yiraeChristineKim force-pushed the ACM-8782 branch from 615a10f to 0063245 Compare January 5, 2024 14:59

yiraeChristineKim requested a review from mprahl January 5, 2024 15:17

Add relatedObjs to unamed mustnothave musthave

bc3883d

Ref: https://issues.redhat.com/browse/ACM-8782 Signed-off-by: Yi Rae Kim <yikim@redhat.com>

yiraeChristineKim force-pushed the ACM-8782 branch from 0063245 to bc3883d Compare January 5, 2024 15:18

mprahl approved these changes Jan 5, 2024

View reviewed changes

openshift-ci bot added the lgtm label Jan 5, 2024

JustinKuli approved these changes Jan 5, 2024

View reviewed changes

openshift-ci bot removed the do-not-merge/hold label Jan 5, 2024

openshift-merge-bot bot merged commit 7529c90 into open-cluster-management-io:main Jan 5, 2024
4 checks passed

magic-mirror-bot bot mentioned this pull request Jan 5, 2024

😿 Failed to sync the upstream PRs: #182 stolostron/config-policy-controller#684

Closed

dhaiducek reviewed Jan 5, 2024

View reviewed changes

__debug_bin2514188118

Copy link

Member

dhaiducek Jan 5, 2024

Choose a reason for hiding this comment

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

@yiraeChristineKim I think this file shouldn't have been merged. Can you revert this, please?

yiraeChristineKim mentioned this pull request Feb 29, 2024

Change unnamed related object from * to - #212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add relatedObjs to unnamed musthave object #182

Add relatedObjs to unnamed musthave object #182

yiraeChristineKim commented Dec 4, 2023

JustinKuli Dec 4, 2023

yiraeChristineKim Dec 4, 2023 •

edited

Loading

JustinKuli Dec 4, 2023

yiraeChristineKim Dec 4, 2023 •

edited

Loading

mprahl Dec 11, 2023

yiraeChristineKim Dec 12, 2023 •

edited

Loading

mprahl Dec 14, 2023

yiraeChristineKim Dec 14, 2023

yiraeChristineKim Dec 14, 2023

JustinKuli Dec 19, 2023

yiraeChristineKim Jan 2, 2024

JustinKuli left a comment

mprahl Jan 5, 2024

mprahl Jan 5, 2024

mprahl Jan 5, 2024

mprahl Jan 5, 2024

mprahl left a comment

mprahl left a comment

JustinKuli left a comment

openshift-ci bot commented Jan 5, 2024

dhaiducek Jan 5, 2024

	By("relatedObjects should not exist")
	By("relatedObjects should exist")

	By("Check the reasons of relatedObjects")
	By("Check the reason of the related object")

Add relatedObjs to unnamed musthave object #182

Add relatedObjs to unnamed musthave object #182

Conversation

yiraeChristineKim commented Dec 4, 2023

Choose a reason for hiding this comment

yiraeChristineKim Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiraeChristineKim Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiraeChristineKim Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinKuli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprahl left a comment

Choose a reason for hiding this comment

mprahl left a comment

Choose a reason for hiding this comment

JustinKuli left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 5, 2024

Choose a reason for hiding this comment

yiraeChristineKim Dec 4, 2023 •

edited

Loading

yiraeChristineKim Dec 4, 2023 •

edited

Loading

yiraeChristineKim Dec 12, 2023 •

edited

Loading