feat(set_ns): add kustomize plugin to set a resource's namespace

[mfleader]

• Adds a Kustomize plugin to set a given Resource's Namespace to the Operator owner instance's Namespace.
• Factors out Name validation behavior into plugins.go for reuse in namespace.go.

[VaishnaviHire]

/lgtm

[rhuss]

Some minor comments, feel free to improve on that if you agree or skip it, too (no hard feelings)

[rhuss]

nit: Usually I would not check on literal error message, but that key context components are contained. E.g. here I would check that "namespace" is included for the context and for "empty" as the error kind. Not necessarily the exact wording, that makes the test more robust for refactorings but still captures the essence.

[rhuss]

Maybe the function name should reflect that it's for the stronge validation of a DNS name, and not for all resource. Something like ValidateDNSResourceName or so.

[rhuss]

@mfleader @leseb @VaishnaviHire @skamenan7 @rhdedgar @Elbehery

While working on the PR (pardonnez-mois for hijacking the issue :), I would quickly raise a proposal:

• Adding a lgtm-with-suggestions that a PR reviewer can add when she only has optional suggestions to add, but no hard feelings and don't want to prevent merging the PR. It's just to seed some proposal that are not hard arguments (stuff, that we usually name as nits)
• The author of the PR the can remove that label to trigger an auto-merge (when the other conditions are met, too) or add the suggestions (in this case, a new approval is needed, and hopefully the automation could remove that label on additional change, much like approvals are removed after additional changes)

This allows us to have some more flexibility, moves back some responsibility to the author and hopefully helps in unblocking and speeding-up things.

wdyt ?

[leseb]

I'm not sure how I feel about this. Typically, "nits" still require changes; what I consider "nit" is something small but still needs to be addressed (maybe I got the understanding of "nit" wrong, though).

If no action is required, I would typically start the comment with: "No action required: ...", then leave it up to the PR author to address or not. I might still leave an approval, also. This happened recently with @mfleader, I had a "no action required" item, approved the PR, and it got merged. @mfleader then followed up with another PR because he felt the item made sense. If I had a strong opinion I wouldn't have had approved the PR in the first place.

Also btw, approvals are not reset on push, we haven't configured that yet, and I'm all for it even though it could slow us down.

All that said, I'm not sure if this label should block merging (I know it's the entire point of the label) :). It feels like more burden to the author to manage the label :)

[rhdedgar]

Interesting, I could see striking a balance between the two options. Nits could still require changes, and lgtm-with-suggestions could be for non-required suggestions.

That does raise the question, do we want to use the lgtm-with-suggestions label only for very small things that won't require a re-review? Example: changing the wording on some print / log output.

Otherwise, there might be some impactful code that gets pushed after the review and approval.

If it's only for small things, I do think it would be helpful to have some kind of label to convey "I approve the PR, but want to give the author time to address optional suggestions". Then the author can merge it later, even if the approver / reviewer isn't still online at the time.

[mfleader]

I agree with @leseb on their points, except I did not think nits had to be addressed, but I'm also happy to accept their definition. Additionally, I think it is time for this conversation to be moved from this pr b/c it is now blocking the conclusion and merging of this pr.