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

chore: add new label to reserved key list #606

Closed
wants to merge 1 commit into from

Conversation

bthari
Copy link
Contributor

@bthari bthari commented Sep 12, 2024

Description

Previously I added a new kubernetes label in #604, this should've been added to reserved key list

Checklist

  • Added PR label
  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduces API changes

Release Notes


@bthari bthari added the maintenance Dependency updates and other chores label Sep 12, 2024
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.70%. Comparing base (a9184ed) to head (52d16c1).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #606   +/-   ##
=======================================
  Coverage   60.70%   60.70%           
=======================================
  Files         275      275           
  Lines       22171    22171           
=======================================
  Hits        13460    13460           
  Misses       7847     7847           
  Partials      864      864           
Flag Coverage Δ
api-test 58.68% <ø> (ø)
sdk-test-3.10 75.51% <ø> (ø)
sdk-test-3.8 75.49% <ø> (ø)
sdk-test-3.9 75.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bthari bthari marked this pull request as ready for review September 12, 2024 06:37
Copy link
Contributor

@deadlycoconuts deadlycoconuts left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 Thanks for the changes!

@deadlycoconuts
Copy link
Contributor

Actually my bad, I suddenly thought about this again, and was thinking that the reserved key list might not actually be used to filter out the managed label at all because that label is only applied on namespaces right?

I don't think we allow users to make changes on the namespace labels so we might not actually need to add this to the reserved list after all. We might need to double check that the reserved list is only used to verify labels that users can change (and if that's the case we don't need to add this managed label anymore since users can't change namespace labels).

@bthari
Copy link
Contributor Author

bthari commented Sep 13, 2024

Now that you mentioned it, @deadlycoconuts, what I need to be clarified is what's the meaning of "label that user can change"? Is it the one with the prefix, e.g. prefix/app?

My confusion comes from the inconsistency here (which as you've mentioned, will not work anyway 😅), where we validate is it reserved based on prefix + key:

if labeller.IsReservedKey(labeller.GetPrefix() + label.Key) {
	continue
}

but on here, when we actually put the name into the label, it wouldn't use the prefix

labels[label.Key] = label.Value

So, I'm thinking that maybe instead of just the app, stream, etc label on the reservedKeys, it should've contained the complete name of our label (e.g. prefix/steam) so then we can also add prefix/managed to the list?

But all in all, I think it's okay to close this merge and not add the managed label to the list, then we can revisit this reservedKeys thing later 😂

@bthari bthari closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Dependency updates and other chores
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants