Use "dep" to import dependencies for generated project #136
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| func RunVendorInstall(cmd *cobra.Command, args []string) { | ||
| if Update { | ||
| backupFilename := fmt.Sprintf("%s.bkp", depManifestFile) | ||
| if err := copyFile(depManifestFile, backupFilename); err != nil { |
There was a problem hiding this comment.
Maybe you can use os.Rename here. Base on the code change, it is OK to not keep the original file.
There was a problem hiding this comment.
Done. Thanks for the suggestion.
| func updateDepConfig() error { | ||
| var depConstraint string | ||
| // template for dep's manifest file (Gopkg.toml). This is generated using | ||
| // scripts/generate_dep_manifest.sh scripts. |
There was a problem hiding this comment.
Just want to make things clear. If we update the dependency of kubebuilder itself, do we need to rerun the script generate_dep_manifest.sh to regenerate this template? So that the dep in generated projects is in sync with kubebuilder itself.
There was a problem hiding this comment.
Good question. Maybe we need a script to wrap dep ensure?
There was a problem hiding this comment.
@Liujingfang1 yes, we should re-run the generate_dep_manifest.sh whenever we are updating kubebuilder dependencies. In some instances, our tests will catch that (it did for the docs change that Phil pushed and I updated it).
So for now, I have made it very simple to do it and added the documentation in the script itself.
| # | ||
|
|
||
| required=[ | ||
| "sigs.k8s.io/testing_frameworks/integration", |
There was a problem hiding this comment.
Lets do these as naked imports so users can manage their own required
| // scripts/generate_dep_manifest.sh scripts. | ||
| const depManifestTmpl = ` | ||
| # | ||
| # Note: Stanzas below are generated by Kubebuilder. |
There was a problem hiding this comment.
# Users add deps lines here# Note: Stanzas below are generated by Kubebuilder and may be rewritten when upgrading kubebuilder versions.
# DO NOT MODIFY BELOW THIS LINE.
| [[constraint]] | ||
| version = "%s" | ||
| name = "github.com/kubernetes-sigs/kubebuilder" |
| version="1.1.3" | ||
|
|
||
| [[override]] | ||
| name="github.com/kubernetes-sigs/kubebuilder" |
There was a problem hiding this comment.
Don't hard code this revision, make it the version or branch
| @@ -126,19 +125,6 @@ function build_kb { | |||
| go build -o $tmp_root/kubebuilder/bin/kubebuilder-gen ./cmd/kubebuilder-gen | |||
| } | |||
|
|
|||
| function prepare_vendor_deps { | |||
There was a problem hiding this comment.
Where will get get deps from once this is merged?
| name = "github.com/kubernetes-sigs/kubebuilder" | ||
| `, kbVersion) | ||
| } | ||
| {{ if eq .Version "unknown" -}} |
There was a problem hiding this comment.
This should just be an override.
|
|
||
| [prune] | ||
| go-tests = true | ||
| #unused-packages = true |
There was a problem hiding this comment.
Lets put this above the generated stanza to allow users to control it.
| var depConstraint string | ||
| // template for dep's manifest file (Gopkg.toml). This is generated using | ||
| // scripts/generate_dep_manifest.sh scripts. | ||
| const depManifestTmpl = ` |
There was a problem hiding this comment.
Maybe this should go in its own file so it is easier to generate and update from script.
There was a problem hiding this comment.
Moved it to separate file.
| fr, err := os.Open(f) | ||
| if err != nil { | ||
| log.Fatalf("failed to read vendor tar file %s %v", f, err) | ||
| func RunVendorInstall(cmd *cobra.Command, args []string) { |
There was a problem hiding this comment.
We should check that dep is installed and give an error message if it is not.
There was a problem hiding this comment.
Thanks for the review @Liujingfang1 @pwittrock . Addressed the review comment. PTAL.
| func updateDepConfig() error { | ||
| var depConstraint string | ||
| // template for dep's manifest file (Gopkg.toml). This is generated using | ||
| // scripts/generate_dep_manifest.sh scripts. |
There was a problem hiding this comment.
@Liujingfang1 yes, we should re-run the generate_dep_manifest.sh whenever we are updating kubebuilder dependencies. In some instances, our tests will catch that (it did for the docs change that Phil pushed and I updated it).
So for now, I have made it very simple to do it and added the documentation in the script itself.
| var depConstraint string | ||
| // template for dep's manifest file (Gopkg.toml). This is generated using | ||
| // scripts/generate_dep_manifest.sh scripts. | ||
| const depManifestTmpl = ` |
There was a problem hiding this comment.
Moved it to separate file.
| // scripts/generate_dep_manifest.sh scripts. | ||
| const depManifestTmpl = ` | ||
| # | ||
| # Note: Stanzas below are generated by Kubebuilder. |
| # | ||
|
|
||
| required=[ | ||
| "sigs.k8s.io/testing_frameworks/integration", |
| name = "github.com/kubernetes-sigs/kubebuilder" | ||
| `, kbVersion) | ||
| } | ||
| {{ if eq .Version "unknown" -}} |
| version="1.1.3" | ||
|
|
||
| [[override]] | ||
| name="github.com/kubernetes-sigs/kubebuilder" |
|
|
||
| [prune] | ||
| go-tests = true | ||
| #unused-packages = true |
| fr, err := os.Open(f) | ||
| if err != nil { | ||
| log.Fatalf("failed to read vendor tar file %s %v", f, err) | ||
| func RunVendorInstall(cmd *cobra.Command, args []string) { |
| func RunVendorInstall(cmd *cobra.Command, args []string) { | ||
| if Update { | ||
| backupFilename := fmt.Sprintf("%s.bkp", depManifestFile) | ||
| if err := copyFile(depManifestFile, backupFilename); err != nil { |
There was a problem hiding this comment.
Done. Thanks for the suggestion.
| @@ -0,0 +1,198 @@ | |||
| /* | |||
| Copyright 2017 The Kubernetes Authors. | |||
There was a problem hiding this comment.
Do you need to change 2017 to 2018?
zh-translation: docs/book/src/cronjob-tutorial/main-revisited.md
Fixes #134