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

fix: Remove kubectl binary from argo image(#5005) #5101

Merged
merged 1 commit into from Jan 21, 2021

Conversation

iam-veeramalla
Copy link
Member

Signed-off-by: iam-veeramalla abhishek.veeramalla@gmail.com

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • My build is green (troubleshooting builds).

@codecov-io
Copy link

codecov-io commented Dec 21, 2020

Codecov Report

Merging #5101 (ed4ad4b) into master (e26ad30) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5101      +/-   ##
==========================================
- Coverage   40.90%   40.88%   -0.02%     
==========================================
  Files         136      136              
  Lines       18419    18426       +7     
==========================================
  Hits         7534     7534              
- Misses       9810     9816       +6     
- Partials     1075     1076       +1     
Impacted Files Coverage Δ
server/project/project.go 55.46% <0.00%> (-0.92%) ⬇️
cmd/util/app.go 33.43% <0.00%> (-0.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e26ad30...ed4ad4b. Read the comment docs.

@iam-veeramalla
Copy link
Member Author

@jannfis @jessesuen This is as part of our discussion at #5005

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also remove hack/installers/install-kubectl-linux.sh

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is incomplete, because the last place where we still use kubectl is at the version endpoint:

See: https://github.com/argoproj/argo-cd/blob/master/server/version/version.go#L32-L51

Can you also update version.go to somehow report the version that we import in go modules?

@iam-veeramalla
Copy link
Member Author

iam-veeramalla commented Dec 22, 2020

This change is incomplete, because the last place where we still use kubectl is at the version endpoint:

See: https://github.com/argoproj/argo-cd/blob/master/server/version/version.go#L32-L51

Can you also update version.go to somehow report the version that we import in go modules?

If we decide to remove kubectl from the image, why should we import or print the version ? @jessesuen

@alexmt
Copy link
Collaborator

alexmt commented Dec 22, 2020

If we decide to remove kubectl from the image, why should we import or print the version ? @jessesuen

Was trying to find a related discussion but looks like it is gone in slack history. Argo CD still uses kubectl as a library. So user still need to know which version it is e.g. to determine if the target k8s cluster compatible or not.

@jessesuen
Copy link
Member

We still care about the version of library because it's intended to be equivalent in behavior to the corresponding kubectl binary (including its bugs and fixes)

@jannfis
Copy link
Member

jannfis commented Jan 12, 2021

Since we use a version pin on the Kubernetes libraries in go.mod (in the replace) section, I think the version could be figured out as follows (Kubernetes libraries use a weird versioning scheme, but effectively v0.20.1 is v1.20.1, v0.18.6 is v1.18.6 etc):

awk '/k8s.io\/client-go => k8s.io\/client-go/ { print $4 }' go.mod | sed -e 's/v0\.\(.*\)/v1\.\1/'

I.e. on current master code this will yield:

$ awk '/k8s.io\/client-go => k8s.io\/client-go/ { print $4 }' go.mod | sed -e 's/v0\.\(.*\)/v1\.\1/'
v1.20.1

We can pass this in Makefile via linker flags to the build process of the binaries, like we already do with the Argo CD version and Git commit SHA, as seen here:

override LDFLAGS += \

Then use this version identifier in the version endpoint, and get rid of kubectl for good.

Btw, there is a version discrepancy in client libs used and kubectl installed in the image currently, so the version information is quite misleading currently.

@iam-veeramalla iam-veeramalla force-pushed the removekubectl branch 3 times, most recently from ec24631 to 52eff1c Compare January 19, 2021 16:46
Makefile Outdated
@@ -14,6 +14,7 @@ GIT_TAG=$(shell if [ -z "`git status --porcelain`" ]; then git describe --exact-
GIT_TREE_STATE=$(shell if [ -z "`git status --porcelain`" ]; then echo "clean" ; else echo "dirty"; fi)
PACKR_CMD=$(shell if [ "`which packr`" ]; then echo "packr"; else echo "go run github.com/gobuffalo/packr/packr"; fi)
VOLUME_MOUNT=$(shell if test "$(go env GOOS)" = "darwin"; then echo ":delegated"; elif test selinuxenabled; then echo ":delegated"; else echo ""; fi)
KUBECTL_VERSION=$(shell awk '/k8s.io\/client-go => k8s.io\/client-go/ { print $4 }' go.mod | sed -e 's/v0\.\(.*\)/v1\.\1/')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to GNU make's own variable expansion, print $4 should be print $$4 here so that the identifier will be processed correctly by awk

@iam-veeramalla iam-veeramalla force-pushed the removekubectl branch 2 times, most recently from 94242ee to ed4ad4b Compare January 20, 2021 14:50
Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
@iam-veeramalla
Copy link
Member Author

Please also remove hack/installers/install-kubectl-linux.sh

Please review

@iam-veeramalla
Copy link
Member Author

This change is incomplete, because the last place where we still use kubectl is at the version endpoint:

See: https://github.com/argoproj/argo-cd/blob/master/server/version/version.go#L32-L51

Can you also update version.go to somehow report the version that we import in go modules?

Please check

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Awesome! Minus 42 MB from the image.

@alexmt alexmt dismissed jessesuen’s stale review January 21, 2021 17:49

requested change was implemented

@alexmt alexmt merged commit eaf9887 into argoproj:master Jan 21, 2021
shubhamagarwal19 pushed a commit to shubhamagarwal19/argo-cd that referenced this pull request Apr 15, 2021
)

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants