Skip to content
This repository was archived by the owner on Apr 17, 2025. It is now read-only.

Inclusive configuration of resources to propagate #217

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Inclusive configuration of resources to propagate #217

merged 1 commit into from
Aug 29, 2022

Conversation

mzeevi
Copy link
Contributor

@mzeevi mzeevi commented Jul 24, 2022 •
edited
Loading

Addresses #16. As suggested in the issue, an additional SyncornizationMode called Allow is added (and not AllowPropagate to allow the kubetcl plugin keep the same logic of converting the strings to Title).

Allow only allows propagation of objects when a selector is set. When a selector is set, Allow follows the same logic as Propagate. When no selectors are set, the object is not propagated.

In addition, an all selector is added.

New unit testing and e2e testing were made for this mode and for the new selector.

The documentation and quickstart testing are left untouched.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 24, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: mzeevi / name: mzeevi (adde9f0)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 24, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @mzeevi!

It looks like this is your first PR to kubernetes-sigs/hierarchical-namespaces 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/hierarchical-namespaces has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 24, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @mzeevi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2022
@mzeevi mzeevi force-pushed the inclusiveConfig branch from 1ef333f to adde9f0 Compare July 24, 2022 14:43
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 24, 2022
adrianludwin
adrianludwin reviewed Aug 9, 2022
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

This looks great, just a bunch of mainly stylistic changes. Thanks for this!

/cc @rjbez17
/ok-to-test

@k8s-ci-robot k8s-ci-robot requested a review from rjbez17 August 9, 2022 14:06
@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 9, 2022
@mzeevi
Copy link
Contributor Author

mzeevi commented Aug 14, 2022

Thanks for the notes, Adrian! I will address everything in a new commit.

@mzeevi
Copy link
Contributor Author

mzeevi commented Aug 23, 2022

I believe I've addressed everything, please take a look @adrianludwin

Thanks!

@mzeevi mzeevi marked this pull request as draft August 23, 2022 16:42
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2022
@mzeevi mzeevi marked this pull request as ready for review August 23, 2022 19:40
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2022
@k8s-ci-robot k8s-ci-robot requested a review from srampal August 23, 2022 19:41
@mzeevi
Copy link
Contributor Author

mzeevi commented Aug 23, 2022

Should be fine now

adrianludwin
adrianludwin reviewed Aug 24, 2022
View reviewed changes
@mzeevi mzeevi force-pushed the inclusiveConfig branch 2 times, most recently from f4002b5 to 813daa9 Compare August 24, 2022 13:43
@mzeevi
Copy link
Contributor Author

mzeevi commented Aug 24, 2022

Made some changes as per your comments, @adrianludwin

Thanks again!

adrianludwin
adrianludwin reviewed Aug 25, 2022
View reviewed changes
@adrianludwin
Copy link
Contributor

lgtm after these last changes are made. Ryan, please feel free to approve (or else I'll do it).

/assign @rjbez17

@mzeevi
Copy link
Contributor Author

mzeevi commented Aug 25, 2022

Made some changes in accordance to the latest comments

adrianludwin
adrianludwin reviewed Aug 29, 2022
View reviewed changes
test/e2e/issues_test.go Outdated
Comment on lines 387 to 388

MustRun("kubectl hns config set-resource secrets --mode Ignore")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't notice this until now - if the test fails, this line will never run. Can you add it to the BeforeEach() function on line 23 of this file, and mention this test as the reason why this needs to be reset? That's my last comment, promise :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, and all comments are welcome :)

@mzeevi
Inclusive configuration of resources to propagate
6f0aca2 
See issue kubernetes-retired#16. To allow inclusive propagation of resources an
additional `SyncornizationMode` called 'AllowPropagate' which only enables
propagation when a selector is set is added. An 'all' selector is also addded.

Tested: e2e-testing covering secrets resource in 'AllowPropagate' mode and checking
propagation when selectors are set and unset ('select', 'treeSelect', 'none', 'all').
Unit testing is also modified to account for the new 'all' selection

Signed-off-by: mzeevi <[email protected]>
@mzeevi
Copy link
Contributor Author

mzeevi commented Aug 29, 2022

Should be fine now @adrianludwin

adrianludwin
adrianludwin reviewed Aug 29, 2022
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks for this change!
I'd appreciate it if you could also submit a docs change afterwards. Here are the sections that should be updated:

Please indicate that these features will be beta in v1.1. Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, mzeevi

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit ec50308 into kubernetes-retired:master Aug 29, 2022
@adrianludwin adrianludwin added this to the release-v1.1 milestone Aug 29, 2022
@adrianludwin adrianludwin mentioned this pull request Aug 29, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adrianludwin adrianludwin adrianludwin left review comments

@tashimi tashimi Awaiting requested review from tashimi

@rjbez17 rjbez17 Awaiting requested review from rjbez17

@srampal srampal Awaiting requested review from srampal

Assignees

@rjbez17 rjbez17

@adrianludwin adrianludwin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
release-v1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@mzeevi @k8s-ci-robot @adrianludwin @rjbez17