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: provide nameNormalized param for local cluster in ApplicationSet cluster generator (#9727) #9728
fix: provide nameNormalized param for local cluster in ApplicationSet cluster generator (#9727) #9728
Conversation
Signed-off-by: Lawrence Carvalho <lawrence.carvalho@sky.uk>
d17b062
to
3aab82d
Compare
Codecov Report
@@ Coverage Diff @@
## master #9728 +/- ##
=======================================
Coverage 45.79% 45.79%
=======================================
Files 227 227
Lines 26920 26921 +1
=======================================
+ Hits 12327 12328 +1
Misses 12911 12911
Partials 1682 1682
Continue to review full report at Codecov.
|
bbafc21
to
497fcab
Compare
@crenshaw-dev any idea when someone will be able to review this? i resolved a conflict yesterday, so would be good to get it in soon! |
Hi @lacarvalho91 , Thanks for PR, but this PR doesn't looks good to me
imho, this changes looks redundant |
@rishabh625 the description in the linked issue should properly explain the motivation but this is simply to provide |
I got you @lacarvalho91 , that's my worry u just need a nameNormalized param which contains value similar to name. I feel let's include that param but atleast populate normalised value in it. @crenshaw-dev what's your opinion? |
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.
@rishabh625 thanks for the ping! lgtm. I'm gonna treat this as a bugfix so we can get it into 2.4.x.
Ok @crenshaw-dev , if u feel PR is fine then good , but I still feel to add a Normalised name as value instead of just name Thanks for merging |
@rishabh625 I think @lacarvalho91 is correct that the name is already normalized by definition because it is always |
Ohh, i thought if we change the name of in-cluster, it still picks the local cluster as local one , it distinguishs local and remote by label and not name. |
@rishabh625 I don't think the local cluster name comes from a secret at all. I think it comes from here:
Is it possible to configure the local cluster via secret? I admit, I'm not super familiar with this area of the code. :-) |
Closes #9727
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist:
I don't believe this requires documentation updates.