-
Notifications
You must be signed in to change notification settings - Fork 147
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
[master] deb,rpm: use cobra generated completions #1150
base: master
Are you sure you want to change the base?
Conversation
deb/common/rules
Outdated
cd /go/src/github.com/docker/cli \ | ||
&& VERSION=$(VERSION) GITCOMMIT=$(CLI_GITCOMMIT) LDFLAGS='' GO_LINKMODE=dynamic ./scripts/build/binary \ | ||
&& DISABLE_WARN_OUTSIDE_CONTAINER=1 LDFLAGS='' make manpages | ||
&& build/docker completion bash > contrib/completion/bash/docker \ | ||
&& build/docker completion zsh > contrib/completion/zsh/_docker \ | ||
&& build/docker completion fish > contrib/completion/fish/docker.fish |
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.
I'm not sure if it's an issue, but the archived source (tar.gz) of the CLI won't contain the generated files and only the generated .deb
package.
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.
Thanks! Change itself looks pretty straightforward (so, happy to see that part wasn't too complicated)
I'm not sure if it's an issue, but the archived source (tar.gz) of the CLI won't contain the generated files and only the generated .deb package.
I don't think we currently publish the source tarballs (at least not for deb
), but if we did, then it's expected for those to container the source files used to build the packages; in this case the completion scripts are build artifacts (similar to how the binaries are built from source), so it's correct for those to not be in the source tar.
On thing I do want to have a look at is if we should write these to contrib/completion/bash/docker
(etc). I think currently this means we're modifying the source code used, so on repeat builds, the git
repository would stay in an unclean ("dirty") state.
Ideally these builds would be able to run with the git source mounted read-only, but we already have some messy paths (e.g. generated man-pages and markdown-docs are written in some subdirectories); ultimately we should look if we can make all produced files end up in build/
(which is the default location for binaries we build).
Perhaps for these, we could create a directory under build/
to store the generated files, and adjust the path that the "install" step looks for to copy the files from;
docker-ce-packaging/deb/common/rules
Lines 69 to 73 in 5029f6d
override_dh_auto_install: | |
# docker-ce-cli install | |
install -D -m 0644 cli/contrib/completion/fish/docker.fish debian/docker-ce-cli/usr/share/fish/vendor_completions.d/docker.fish | |
install -D -m 0644 cli/contrib/completion/zsh/_docker debian/docker-ce-cli/usr/share/zsh/vendor-completions/_docker | |
install -D -m 0755 cli/build/docker debian/docker-ce-cli/usr/bin/docker |
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.
I need to give this PR a try locally (also to confirm the "dirty state" bit; I THINK that would be the case because (sigh!) this repository still uses a bind-mount to mount the source in, so state is mutable).
Otherwise; looking good already! (Thanks!)
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.
I need to give this PR a try locally (also to confirm the "dirty state" bit;
Quick update on that part; I was wrong on it leaving a "dirty" state on the host, because we build a tgz
that's used for the deb/rpm build tools.
That said; I'm working on a local branch to explore some options, and I think I got that working
7b03403
to
ac85b61
Compare
Co-authored-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Alano Terblanche <[email protected]>
ac85b61
to
6c19ca1
Compare
I updated this one with the changes from #1156 (using the new Makefile target from docker/cli#5770) |
- What I did
Replace the CLI completions (located in
cli/contrib/completions
with cobra generated ones.- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)