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

Target selectors docs #2534

Merged
merged 14 commits into from
Oct 13, 2021
Merged

Conversation

phanimarupaka
Copy link
Contributor

No description provided.

@phanimarupaka phanimarupaka force-pushed the TargetSelectorsDocs branch 3 times, most recently from b8f99f0 to a64b67c Compare October 6, 2021 20:51
4. `namespace`: `metadata.namespace` field of resources to be selected.
5. `packagePath`: [Package identifier] of resources to be selected. Examples values:
- `.` - selects resources in current package excluding resources in subpackages of current package.
- `mysql` - selects resources in the `mysql` subpackage excluding resources of nested subpackages of `mysql`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this counter-intuitive. I expected specifying mysql would include its subpackages. It's probably a more common use case to want to select the entire package hierarchy under mysql, no? With this model, how do you specify that? Is the thinking that there'll be a wildcard syntax?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed about this and the point is about the default behavior and what makes it more intuitive to users. How about making what you mentioned a default behavior. If users wants to specifically exclude subpackages we can include a new field called excludeSubpackages.

So shall I reimplement this to make it package instead of packagePath and accept full package identifiers with no wild cards support ? Where default behavior is to include all the subpackages?

Copy link
Contributor

Choose a reason for hiding this comment

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

To unblock the rest of the feature, you can leave packagePath out of the documentation, and address it separately. Design discussions are better done in a design doc where you can evaluate the overall UX and consider alternatives. There's also a related discussion about globbing support across all matchers. target field in Kustomize patch support wildcards, has this been discussed anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

So closing the loop here, @phanimarupaka and me had an offline discussion about it. We agree the current behavior is unintuitive, So we are going to disable packagePath selector for now and re-introduce it with along with subpackage exclusion functionality later.

Copy link
Contributor

@frankfarzan frankfarzan left a comment

Choose a reason for hiding this comment

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

Approved after removing packagePath and following up on open questions.

@phanimarupaka phanimarupaka merged commit c16d0c9 into kptdev:main Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants