Refactor name setting for complex cases #174

yiraeChristineKim · 2024-08-01T13:23:23Z

This PR supports the more complex cases of setting names. Especially, consolidateManifests is true and the manifest-path name is present.
https://issues.redhat.com/browse/ACM-1256

yiraeChristineKim · 2024-08-01T13:24:55Z

internal/ordering_test.go

 policies:
 - name: one
  manifests:
+  - path: {{printf "%v/%v" .Dir "configmap.yaml"}}
+    name: tiger
+  - path: {{printf "%v/%v" .Dir "configmap.yaml"}}


Although this manifest path name has "rabbit", this name is ignored because it is consolidateManifests. The configuration name will be "tiger"

It feels like this test has a lot going on, I couldn't quite follow what it was doing. Can it be split into multiple tests? Or does it need to be all together in one?

I understand I wanted to cover most of the edge cases. because base cases are covered in other unit tests

I can say this is the ULTIMATE test case.

Maybe it is good idea to split Let me think

JustinKuli · 2024-08-01T14:52:15Z

internal/ordering_test.go

 policies:
 - name: one
  manifests:
+  - path: {{printf "%v/%v" .Dir "configmap.yaml"}}
+    name: tiger
+  - path: {{printf "%v/%v" .Dir "configmap.yaml"}}


It feels like this test has a lot going on, I couldn't quite follow what it was doing. Can it be split into multiple tests? Or does it need to be all together in one?

JustinKuli · 2024-08-01T14:57:11Z

internal/utils.go

+func getConfigurationPolicyName(policyConf *types.PolicyConfig, manifestGroupIndex int,
+	usedPolicyNameNum *int, usedManifestNameNum *int,


Passing these ints as pointers makes it possible to increment them conditionally, but it seems hard to reason about without looking through this entire function. Maybe it's just too clever for me.

I wonder if returning the updated values would help? Or maybe there needs to be a comment when this function is called.

It is related to previous PR sorry about confusing

Inside the function, we don't know which name is used so

JustinKuli · 2024-08-01T14:59:05Z

internal/utils.go

+					getConfigurationPolicyName(policyConf, i,
+						&usedPolicyNameNum, &usedManifestNameNum),


I wonder if this kind of thing would help me:

Suggested change

getConfigurationPolicyName(policyConf, i,

&usedPolicyNameNum, &usedManifestNameNum),

usedPolicyNameNum, usedManifestNameNum = getConfigurationPolicyName(policyConf, i,

usedPolicyNameNum, usedManifestNameNum),

But it's kind of long, maybe it's not better 😓

dhaiducek

Looks good! The counter is definitely tricky. I've left some comments to consider.

I also keep going back and forth in my mind on whether we should adjust the current numbering system. This is how it should have been implemented originally, but it is a change in behavior and, if we're going to implement this, it shouldn't be backported.

docs/policygenerator-reference.yaml

internal/utils.go

dhaiducek · 2024-08-01T19:38:16Z

internal/utils.go

+	// For the case where ConsolidateManifests is true and the manifest's name is provided,
+	if usedManifestNameNum == nil && configName != "" {
+		return configName
+	}
+
+	if usedManifestNameNum != nil && configName != "" {
+		*usedManifestNameNum++
+		if *usedManifestNameNum > 1 {
+			return fmt.Sprintf("%s%d", configName, *usedManifestNameNum)
 		}

 		return configName
 	}


This could be combined (also incorporates proposed changes to not use manifests.name when consolidateManifests: true)

Suggested change

// For the case where ConsolidateManifests is true and the manifest's name is provided,

if usedManifestNameNum == nil && configName != "" {

return configName

}

if usedManifestNameNum != nil && configName != "" {

*usedManifestNameNum++

if *usedManifestNameNum > 1 {

return fmt.Sprintf("%s%d", configName, *usedManifestNameNum)

}

return configName

}

// The manifest name is provided

if usedManifestNameNum != nil && configName != "" {

*usedManifestNameNum++

if *usedManifestNameNum > 1 {

return fmt.Sprintf("%s%d", configName, *usedManifestNameNum)

}

return configName

}

Should we add error message or warning message for this, otherwise this is really confusing usesr.

Brian commented https://issues.redhat.com/browse/ACM-12563

I don't think it's confusing for the user since the manifests are being combined so it's using the parent configuration, and I think (?) this PR resolves Brian's code bug since the manifest[].name is handled separately for object-templates-raw.

Oh, I see. I wasn't thinking about this allowing configuration of the ConfigurationPolicy to have a different name than the Policy. Yes, this makes sense then to just use the first name it finds.

So leave it as t is?

Leave the logic, yes. I still think the conditionals could optionally be combined:

// The manifest name is provided if configName != "" { // The manifest name counter is not nil so must be incremented and used if it's greater than one if usedManifestNameNum != nil { *usedManifestNameNum++ if *usedManifestNameNum > 1 { return fmt.Sprintf("%s%d", configName, *usedManifestNameNum) } } // Return the manifest name without a suffix return configName }

I might have to think more about whether we should be doing duplicate name validation, but right now I'm thinking that can be chalked up to user error since the framework would complain once it's deployed.

I am not sure this repos has sonarcloud. Sonar doesn't like too many if-if child.

internal/utils.go

docs/policygenerator-reference.yaml

internal/utils.go

https://issues.redhat.com/browse/ACM-12563 Signed-off-by: yiraeChristineKim <[email protected]>

mprahl · 2024-08-02T17:25:00Z

/hold for other reviews

dhaiducek

There are some edge cases here where the same name could be specified throughout for policies[].name and manifests[].name.

Though for policies[].name, it was a blocking thing since the object couldn't be deployed at all so it really needed to be an error case. For manifests[].name, the framework would let the user know that the same name is in use so we could chalk it up to user error and not have it be blocking, which I think I'd be fine with.

I still maintain that the numbering changes should not be backported. If there is a backport, it should only include the portion of this PR dealing with object-templates-raw.

openshift-ci · 2024-08-02T20:23:46Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

yiraeChristineKim · 2024-08-05T18:49:02Z

/unhold

yiraeChristineKim · 2024-08-05T19:01:50Z

There are some edge cases here where the same name could be specified throughout for policies[].name and manifests[].name.

Though for policies[].name, it was a blocking thing since the object couldn't be deployed at all so it really needed to be an error case. For manifests[].name, the framework would let the user know that the same name is in use so we could chalk it up to user error and not have it be blocking, which I think I'd be fine with.

I still maintain that the numbering changes should not be backported. If there is a backport, it should only include the portion of this PR dealing with object-templates-raw.
The Fix version is 2.12 so we don't need to backport Thanks for your comment. I haven't thought about a "same name" case. Good Catch!!
Should we give the use a warning message when manifests[].name and policies[].name are same?

openshift-ci bot added the dco-signoff: yes label Aug 1, 2024

openshift-ci bot requested review from gparvin and JustinKuli August 1, 2024 13:23

openshift-ci bot added the approved label Aug 1, 2024

yiraeChristineKim commented Aug 1, 2024

View reviewed changes

yiraeChristineKim requested review from mprahl and dhaiducek August 1, 2024 13:28

yiraeChristineKim force-pushed the ACM-12563 branch from 802d4cc to 5ad152b Compare August 1, 2024 13:30

JustinKuli reviewed Aug 1, 2024

View reviewed changes

dhaiducek reviewed Aug 1, 2024

View reviewed changes

yiraeChristineKim force-pushed the ACM-12563 branch from 5ad152b to a069b6d Compare August 2, 2024 16:26

mprahl reviewed Aug 2, 2024

View reviewed changes

docs/policygenerator-reference.yaml Outdated Show resolved Hide resolved

mprahl reviewed Aug 2, 2024

View reviewed changes

internal/utils.go Outdated Show resolved Hide resolved

Refactor name setting for complex cases

7516117

https://issues.redhat.com/browse/ACM-12563 Signed-off-by: yiraeChristineKim <[email protected]>

yiraeChristineKim force-pushed the ACM-12563 branch from a069b6d to 7516117 Compare August 2, 2024 17:23

mprahl approved these changes Aug 2, 2024

View reviewed changes

openshift-ci bot assigned mprahl Aug 2, 2024

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

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

dhaiducek approved these changes Aug 2, 2024

View reviewed changes

openshift-ci bot assigned dhaiducek Aug 2, 2024

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

openshift-merge-bot bot merged commit dd1f91f into open-cluster-management-io:main Aug 5, 2024
4 checks passed

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

🤖 Sync from open-cluster-management-io/policy-generator-plugin: #174 stolostron/policy-generator-plugin#99

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor name setting for complex cases #174

Refactor name setting for complex cases #174

yiraeChristineKim commented Aug 1, 2024

yiraeChristineKim Aug 1, 2024

JustinKuli Aug 1, 2024

yiraeChristineKim Aug 1, 2024

yiraeChristineKim Aug 1, 2024

yiraeChristineKim Aug 1, 2024 •

edited

Loading

JustinKuli Aug 1, 2024

JustinKuli Aug 1, 2024

yiraeChristineKim Aug 1, 2024

yiraeChristineKim Aug 1, 2024

JustinKuli Aug 1, 2024

dhaiducek left a comment

dhaiducek Aug 1, 2024

yiraeChristineKim Aug 2, 2024

yiraeChristineKim Aug 2, 2024

dhaiducek Aug 2, 2024

dhaiducek Aug 2, 2024

yiraeChristineKim Aug 2, 2024

dhaiducek Aug 2, 2024

yiraeChristineKim Aug 2, 2024

mprahl commented Aug 2, 2024

dhaiducek left a comment

openshift-ci bot commented Aug 2, 2024

yiraeChristineKim commented Aug 5, 2024

yiraeChristineKim commented Aug 5, 2024

		func getConfigurationPolicyName(policyConf *types.PolicyConfig, manifestGroupIndex int,
		usedPolicyNameNum int, usedManifestNameNum int,

		getConfigurationPolicyName(policyConf, i,
		&usedPolicyNameNum, &usedManifestNameNum),

Refactor name setting for complex cases #174

Refactor name setting for complex cases #174

Conversation

yiraeChristineKim commented Aug 1, 2024

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

yiraeChristineKim Aug 1, 2024 • 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

dhaiducek 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

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 commented Aug 2, 2024

dhaiducek left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 2, 2024

yiraeChristineKim commented Aug 5, 2024

yiraeChristineKim commented Aug 5, 2024

yiraeChristineKim Aug 1, 2024 •

edited

Loading