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

Add filter for apps for which ownership can be overriden #996

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

100mik
Copy link
Contributor

@100mik 100mik commented Aug 5, 2024

What this PR does / why we need it:

Introduces a flag which allows users to specify flags which apps ownership can be snatched from while allowing ownership overrides.

Which issue(s) this PR fixes:

Fixes #993

Introduce flag to filter out apps which we can steal ownership from while deploying

Additional Notes for your reviewer:

The only catch today, is that it is a best effort listing of apps, limited to the namespace the new app itself is being deployed in. This is in line with encouraged kapp-controller patterns.

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


@100mik 100mik force-pushed the filtered-ownership-override branch 7 times, most recently from bf6bc86 to 24a56f5 Compare August 5, 2024 10:38
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM!
I am just trying to think if we can have a better name for the flag, otherwise looks good.

@100mik
Copy link
Contributor Author

100mik commented Aug 5, 2024

Some options I had thought of, ownership-override-allowed-apps, overridable-apps (not a fan), ownership-override-filter
What do you think of these? I quite like the first one, since it is technically not a filter. Filters have mainly been for user supplied resources.

@100mik 100mik force-pushed the filtered-ownership-override branch from 24a56f5 to e69f420 Compare August 5, 2024 13:40
@100mik
Copy link
Contributor Author

100mik commented Aug 5, 2024

From a convo offline, we are going with ownership-override-allowed-apps 🙌🏼

@100mik 100mik force-pushed the filtered-ownership-override branch from e69f420 to 594af4f Compare August 5, 2024 14:00
@100mik
Copy link
Contributor Author

100mik commented Aug 5, 2024

Created #997 and #998 to track discussions that happened while we were working on this

@100mik 100mik force-pushed the filtered-ownership-override branch from 594af4f to 7a62e2b Compare August 5, 2024 14:09
@100mik 100mik merged commit d5db65f into carvel-dev:develop Aug 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Selectively override ownership while using --dangerous-override-ownership-of-existing-resources
2 participants