-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Named arguments for path formatting #1252
Named arguments for path formatting #1252
Conversation
Hi @Adirio. 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 Once the patch is verified, the new status will be reflected by the 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. |
c0d2cb2
to
8a87980
Compare
/assign @mengqiy This is ready to review, tests passed before squashing so they should also pass now. |
/assign @DirectXMan12 |
81cb6e5
to
542a8f7
Compare
This comment has been minimized.
This comment has been minimized.
542a8f7
to
1659794
Compare
1659794
to
2918b7a
Compare
This comment has been minimized.
This comment has been minimized.
Will rebase and update this after #1340 and related PRs |
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.
Took a quick look, have a question.
f661257
to
ecc1e16
Compare
3a0b1a7
to
5210199
Compare
This comment has been minimized.
This comment has been minimized.
5210199
to
b1cf2c0
Compare
b1cf2c0
to
f7261fb
Compare
Rebased. @droot Shall we merge this PR? |
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.
pkg/model/resource/options.go
Outdated
if _, err := os.Stat(file); os.IsNotExist(err) { | ||
if coreDomain, found := coreGroups[opts.Group]; found { | ||
pkg = path.Join("k8s.io", "api", opts.Group, opts.Version) | ||
pkg = replacer.Replace(path.Join("k8s.io", "api", "%[group]s", "%[version]s")) |
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.
Re-reading the code with fresh eyes, I realized, we introduced many new concepts while working through the iterations:
- replacer (strings.Replacer)
- Keys that can be replaced (kind, package-name, group etc...) which now contributors will have to dig through resource.go's Replace function to understand what all is supported.
I am not sure if the benefits are worth introducing this complexity. I know I should have spotted these in my earlier reviews, but didn't, so sorry about that.
One thing I realized, one thing is missing though: quick abstraction to get-the-types-path-dir
to store ..._types.go
and get-the-controller-path-dir
to store ..._controller.go
.
For example, in this function, we need get-the-types-path-dir
in three places, it will help to have a helper just to do that.
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.
The helper issue has been solved, the complexity concern is being discussed.
cffabfc
to
de43fda
Compare
/test pull-kubebuilder-e2e-k8s-1-16-2 |
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.
few minor nits, then we are good to go.
de43fda
to
f35d6b6
Compare
Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>
f35d6b6
to
ec5c4ad
Compare
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: Adirio 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 |
/test pull-kubebuilder-e2e-k8s-1-17-0 |
Description
This PR allows the usage of named-arguments while formatting files' paths.
Resources have a
Replacer
method that returns anstrings.Replacer
that is used to format paths. Thestrings.Replacer
dictionary is as follows:%[group]s
:Resource.Group
%[group-package-name]s
:Resource.GroupPackageName
%[version]s
:Resource.Version
%[kind]s
:strings.ToLower(Resource.Kind)
%[plural]s
:strings.ToLower(Resource.Plural)
Motivation
Despite the fact that the path can be provided to these file structs, access to some of the fields is restricted to the default path. This formatting ("%[kind]s" getting replaced by strings.ToLower(resource.Kind)) does not only apply to default paths. Paths provided outside the GetInputfunction do also benefit from it.
For example, if you want the types file to be inside a directory of its version, this can be done in the default package because you have access to the resource, but it can't be done before that. By providing this path temnplating options paths can be configured before having access to the exact resource. This allows to provide paths before having access to the resource like in a flag (for implementing a flag that is in kubebuilder` but breaks the scaffolding right now, the boilerplate path.
This PR is part of a bigger change tracked in #1218 but can be applied rightaway.