Skip to content

Refine documentation for fetch.git.subpath #1061

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Refine documentation for fetch.git.subpath #1061

wants to merge 3 commits into from

Conversation

gberche-orange
Copy link

What this PR does / why we need it:

Refine documentation for fetch.git.subpath

To remove ambiguity that the root would remain the same but only the subpath is preserved (i.e users could incorrectly think absolute paths within the repo would be preserved)

See related code:

kapp-controller/pkg/fetch/vendir.go

Line 157 in ad24bdc

NewRootPath: git.SubPath,

Does this PR introduce a user-facing change?

Refine documentation for fetch.git.subpath

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:

@gberche-orange gberche-orange mentioned this pull request Jan 20, 2023
Open
@neil-hickey
Copy link

neil-hickey commented Jan 23, 2023 •
edited
Loading

Hey @gberche-orange ! Thanks for the PR !

The way that kapp-controller is structured is that when we make changes to a CRD definition file, we then have to generate the relevant protobuf file. So you will need to do the following on this PR to get it green.

Please run './hack/build.sh', './hack/gen.sh', and './hack/gen-apiserver.sh' and 'git add' the generated file(s).

You will also have to update your commits to include a "Signed-off by" git commit --signoff message. Clicking the failing DCO workflow will present you instructions on how to do this.

@gberche-orange gberche-orange mentioned this pull request Jan 24, 2023
Closed
5 tasks
@gberche-orange
Copy link
Author

gberche-orange commented Jan 24, 2023 •
edited
Loading

thanks @neil-hickey
I've included doc updates from #1051 and pushed the updated CRDs

@neil-hickey
Copy link

@gberche-orange looks like you might have to re-run the generators one more time. I can probably get around to it over the next couple of days, but if you get a spare couple of minutes please feel free to update and I can get this merged asap! TY

@neil-hickey neil-hickey added carvel-accepted This issue should be considered for future work and that the triage process has been completed awaiting-input labels Feb 22, 2023
@gberche-orange
Copy link
Author

gberche-orange commented Feb 24, 2023 •
edited
Loading

@neil-hickey Sorry I had missed the failed output of

time ./hack/gen-apiserver.sh 
[...]
+ protoc --version
./hack/gen-apiserver.sh: line 72: protoc: command not found
+ rm -rf /tmp/tmp.ylfcLC9LMr
+ git checkout vendor
Updated 1 path from the index

real    0m58,759s
user    0m39,873s
sys     0m14,302s

I searched for a while the automated install of protoc in the github workflows, before understanding protoc binary should be installed manually (as linux binary in my case):

kapp-controller/hack/gen-apiserver.sh

Lines 68 to 72 in 8d8de5c

# Install protoc binary as directed by https://github.com/gogo/protobuf#installation
# (Chosen: https://github.com/protocolbuffers/protobuf/releases/download/v3.0.2/protoc-3.0.2-osx-x86_64.zip)
# unzip archive into ./tmp/protoc-dl directory
export PATH=$PWD/tmp/protoc-dl/bin/:$PATH
protoc --version

The current build process should likely include the protocol buffer generation, and not require contributors to build the .proto and commit them. This makes the contribution process slow and unreliable. This is also likely making the build non reproductible. https://slsa.dev/spec/v0.1/levels#summary-of-levels

Details of the steps I took on my box for contributing the updated .proto file

``` $ history

1001 time ./hack/build.sh
1002 time ./hack/gen.sh
1003 time ./hack/gen-apiserver.sh
1004 time ./hack/install-deps.sh
1005 less ./hack/install-deps.sh
1006 sudo chown guillaume:guillaume /usr/local/bin/ytt
1007 time ./hack/install-deps.sh
1008 sudo chown guillaume:guillaume /usr/local/bin/kbld
1009 time ./hack/install-deps.sh
1010 sudo chown guillaume:guillaume /usr/local/bin/vendir
1011 time ./hack/install-deps.sh
1012 sudo chown guillaume:guillaume /usr/local/bin/kapp
1013 time ./hack/install-deps.sh
1014 time ./hack/gen-apiserver.sh
1015 curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v3.0.2/protoc-3.0.2-osx-x86_64.zip
1016 rm protoc-3.0.2-osx-x86_64.zip
1017 curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v3.0.2/protoc-3.0.2-linux-x86_64.zip
1018 mkdir /tmp/protoc-dl
1019 rmdir /tmp/protoc-dl
1020 mkdir -p ./tmp/protoc-dl
1021 unzip protoc-3.0.2-linux-x86_64.zip ./tmp/protoc-dl/
1022 cd ./tmp/protoc-dl
1023 unzip ../../protoc-3.0.2-linux-x86_64.zip
1024 time ./hack/gen-apiserver.sh
1025 cd ../..
1026 time ./hack/gen-apiserver.sh

$ time ./hack/build.sh

  • export CGO_ENABLED=0
  • CGO_ENABLED=0
  • go mod vendor
  • go mod tidy
  • go fmt ./cmd/... ./pkg/...
  • go build -trimpath -mod=vendor -o controller ./cmd/controller/...
  • ls -la ./controller
    -rwxrwxr-x 1 guillaume guillaume 75520998 févr. 24 08:42 ./controller
  • ./hack/gen-crds.sh
  • ytt -f config/
  • go test --exec=echo ./...
  • echo SUCCESS
    SUCCESS

real 7m22,883s
user 5m46,242s
sys 1m19,115s

time ./hack/gen.sh

  • source hack/utils.sh
    ++ go_mod_gopath_hack
    +++ mktemp -d
    ++ local tmp_dir=/tmp/tmp.Asu0H8cfLO
    +++ go list -m
    ++ local module=github.com/vmware-tanzu/carvel-kapp-controller
    ++ local tmp_repo=/tmp/tmp.Asu0H8cfLO/src/github.com/vmware-tanzu/carvel-kapp-controller
    +++ dirname /tmp/tmp.Asu0H8cfLO/src/github.com/vmware-tanzu/carvel-kapp-controller
    ++ mkdir -p /tmp/tmp.Asu0H8cfLO/src/github.com/vmware-tanzu
    ++ ln -s /home/guillaume/public-code/carvel-kapp-controller /tmp/tmp.Asu0H8cfLO/src/github.com/vmware-tanzu/carvel-kapp-controller
    ++ echo /tmp/tmp.Asu0H8cfLO
  • export GOPATH=/tmp/tmp.Asu0H8cfLO
  • GOPATH=/tmp/tmp.Asu0H8cfLO
  • trap 'rm -rf /tmp/tmp.Asu0H8cfLO' EXIT
  • KC_PKG=github.com/vmware-tanzu/carvel-kapp-controller
  • rm -rf pkg/client
  • echo 'Generating deepcopy funcs'
    Generating deepcopy funcs
    ++ grep zz_generated.deepcopy.go
    ++ find pkg/apis
  • rm -f pkg/apis/packaging/v1alpha1/zz_generated.deepcopy.go pkg/apis/internalpackaging/v1alpha1/zz_generated.deepcopy.go pkg/apis/kappctrl/v1alpha1/zz_generated.deepcopy.go
  • go run vendor/k8s.io/code-generator/cmd/deepcopy-gen/main.go --input-dirs github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/packaging/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/internalpackaging/v1alpha1 -O zz_generated.deepcopy --bounding-dirs github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis --go-header-file ./hack/gen-boilerplate.txt
  • echo 'Generating clientset'
    Generating clientset
  • go run vendor/k8s.io/code-generator/cmd/client-gen/main.go --clientset-name versioned --input-base '' --input github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/packaging/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/internalpackaging/v1alpha1 --output-package github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/clientset --go-header-file ./hack/gen-boilerplate.txt
  • echo 'Generating listers'
    Generating listers
  • go run vendor/k8s.io/code-generator/cmd/lister-gen/main.go --input-dirs github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/packaging/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/internalpackaging/v1alpha1 --output-package github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/listers --go-header-file ./hack/gen-boilerplate.txt
  • echo 'Generating informers'
    Generating informers
  • go run vendor/k8s.io/code-generator/cmd/informer-gen/main.go --input-dirs github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/packaging/v1alpha1,github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/internalpackaging/v1alpha1 --versioned-clientset-package github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/clientset/versioned --listers-package github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/listers --output-package github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/informers --go-header-file ./hack/gen-boilerplate.txt
  • echo GEN SUCCESS
    GEN SUCCESS
  • rm -rf /tmp/tmp.Asu0H8cfLO

real 0m24,976s
user 0m18,810s
sys 0m7,535s

[...]


</details>

praveenrewar and others added 3 commits February 24, 2023 10:31
@praveenrewar @gberche-orange
Update hint in error messages (carvel-dev#1040)
0256ebf 
Signed-off-by: Guillaume Berche <[email protected]>
@kumaritanushree @gberche-orange
Updated repo url and package name to generic name in examples (carvel…
c757719 
…-dev#1039)

* updating repo url to generic name as TCE is getting deprecated and updated package name to a generic name as well

* updated namespace flag to package available cmd

* removed -n flag from cmd added in example

* reverted unwanted change

Co-authored-by: kumari tanushree <[email protected]>
Signed-off-by: Guillaume Berche <[email protected]>
@gberche-orange
Refine documentation for subpath
3af8f6f 
To remove ambiguity that the root would remain the same but only the subpath is preserved (i.e users could incorrectly think absolute paths within the repo would be preserved)

See related code:
https://github.com/carvel-dev/kapp-controller/blob/ad24bdc3e41219b176f88280a469e9fd21979339/pkg/fetch/vendir.go#L157-L157

Signed-off-by: Guillaume Berche <[email protected]>
@gberche-orange
Copy link
Author

Just forced push with DCO which was missing, sorry

git rebase HEAD~3 --signoff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
awaiting-input carvel-accepted This issue should be considered for future work and that the triage process has been completed
Projects
Carvel
Status: Needs Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gberche-orange @neil-hickey @praveenrewar @kumaritanushree