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

pkg/plugin/v3: use repo instead of directory basename as project name #1596

Merged

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Jul 14, 2020

When I create a project, I want to be able to specify my project name. Furthermore I would like to create/work on my project in an arbitrary directory. Instead of adding a flag to support setting project name, use the basename of --repo which is more likely to be the desired project name than the directory name.

/cc @DirectXMan12 @hasbro17 @camilamacedo86

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 14, 2020
@estroz
Copy link
Contributor Author

estroz commented Jul 14, 2020

/test pull-kubebuilder-e2e-k8s-1-17-0

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jul 14, 2020

Hi @estroz,

If we allow for Kubebuilder the repo be just the project name then how this scenario will work for the templates where is expected have the go path? See for example, https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/v3/scaffolds/internal/templates/main.go#L154 and https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/v2/scaffolds/internal/templates/gomod.go#L47

@estroz
Copy link
Contributor Author

estroz commented Jul 14, 2020

@camilamacedo86 this PR isn't actually changing anything material. IMO the init validation code is incorrectly validating the directory name for DNS 1123 compliance, when it really should be validating the repo path's basename. This PR does not change the repo string itself.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jul 14, 2020

Hi @estroz,

The validation occurs because the dir name is used to create the resources as the namespace. So, if we do not ensure that the project dir which is the project name has a valid DSN name than I believe that issues will be faced after. If I can remember it has an issue raised here in the repo.

@estroz
Copy link
Contributor Author

estroz commented Jul 14, 2020

@camilamacedo86 yes and that's a problem because an operator project can be checked out in an arbitrary directory. This PR fixes that problem because it uses the repo field in a PROJECT file, which is checked into VCS and stable. I will look for that issue.

@camilamacedo86
Copy link
Member

Yep, I catcher your change. It shows fine for me.
/lgtm
/approve

Adding /hold to allow you merge as when you wish.

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, estroz

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:
  • OWNERS [camilamacedo86,estroz]

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 merged commit b335ded into kubernetes-sigs:master Jul 14, 2020
@camilamacedo86
Copy link
Member

The /hold did not work and the prow was not re-trigged as default :-(

@joelanford
Copy link
Member

Does this PR need to be reverted for more discussion?

@pwittrock
Copy link
Contributor

pwittrock commented Jul 16, 2020

@camilamacedo86 @estroz Does this assume a 1:1 repo to project model? Is this fixing the problem, or shifting the problem to a different set of users?

@estroz
Copy link
Contributor Author

estroz commented Jul 16, 2020

@pwittrock my logic for this change is that the basename of a repo path is often more descriptive of a project than the directory name the project was created in, is consistent across git checkouts, and can be DNS 1123-compliant without requiring the directory name be so. Are there scenarios where that isn't true, and directory should be used?

I'm effectively trying to work around the lack of having a "project name" stored somewhere in PROJECT.

@pwittrock
Copy link
Contributor

@estroz I am a bit rusty, thanks for helping with the context. Being able to explicitly specify the project name makes sense to me. Optionally decoupling the namespace from the project name also makes sense. How is the base directory name not consistent across git checkouts -- shouldn't it be the same for each clone of the git repo? Is the following understanding of the changes in this PR correct?

Before: 2 different projects hosted in the same git repo -- different project names

/Users/pwittrock/Documents/my-repo.git/my-project-1/

The project name would be my-project-1

/Users/pwittrock/Documents/my-repo.git/my-project-2/

The project name would be my-project-2

Now: 2 different projects hosted in the same git repo -- same project name

/Users/pwittrock/Documents/my-repo.git/my-project-1/

The project name would be my-repo

/Users/pwittrock/Documents/my-repo.git/my-project-2/

The project name would also be my-repo

@estroz
Copy link
Contributor Author

estroz commented Jul 16, 2020

@pwittrock the git repo path and Go module path (set in the PROJECT's repo field) are two different objects, while often the same. If the two projects in your example have different module paths in their go.mod files, which they likely will, then your project names would match your "Before" case.

@estroz
Copy link
Contributor Author

estroz commented Jul 16, 2020

Also:

  • Is it reasonable to force the directory to be a DNS 1123 label?
  • Perhaps the right solution here is to have a top-level config project or project-name field. Right now only kubebuilder's init command uses this value; what if we want to add/modify post-init commands such that they use a project name? Then the project must be checked out in the same directory to use those commands if kubebuilder were to continue using directory name as project name. Setting a config project value during init would allow checkouts in any directory and makes for cleaner code (field access vs os.Getwd()).

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jul 16, 2020

I liked the idea to add the flag project or project-name + a new config field for that which by default will be the dir-name 👍

@pwittrock
Copy link
Contributor

Asking for the project name makes sense to me. I think either defaulting it to the repo name if unspecified or making it required are fine.

@camilamacedo86
Copy link
Member

Hi, @pwittrock and @estroz,

So, If I understood the consensus here is:

Am I right?

@pwittrock
Copy link
Contributor

pwittrock commented Jul 17, 2020

That sounds good to me. I don't feel strongly about the dir vs repo naming, except that changing it might break existing user's workflows. If we could be reasonably confident that the repo-name was generally preferred by users and was unlikely to unhappily surprise existing users I'd be fine with the repo as a default name. Since this only impacts the init command, it is not going to break existing projects, and would more likely just surprise users.

@estroz WDYT of this -- we provide the flag now, default to the dir, and switch to the default to be the repo for kubebuilder v3?

@estroz
Copy link
Contributor Author

estroz commented Jul 21, 2020

@pwittrock that sounds good to me. This should be a v3 plugin thing too, not an addition to the v2 plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants