-
Notifications
You must be signed in to change notification settings - Fork 43
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
Allow using custom kubectl binary #46
Conversation
Add a `kubectl` attribute to k8s_deploy, allowing to specify a custom kubectl binary. Fix #33 Signed-off-by: Steeve Morin <[email protected]>
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.
My only concern with the change, is that the new k8s_deploy
parameter will need to be supported alongside with the toolchain kubectl configuration (which is planned). Overall LGTM. The are couple change requests, tough:
- Could you please run buildidifer (
bazel run //:buildifier
) to let the CI to go through. - Please update e2e test (
examples/e2e-test.sh
) to rely on the feature you are introducing. You can add customkubectl
noop script to https://github.com/adobe/rules_gitops/blob/master/examples/helloworld/BUILD#L50 - Please document new
k8s_deploy
parameter inREADME.md
I figure I’d get some feedback before going all the way. |
Actually you are right that using a toolchain might be cleaner. I’ll take a look. |
There is another contribution #22 that is left in unmerged state because it is waiting for the toolchain implementation. @michaelschiff is ok with using a fork for now. I'm not objecting to merging this PR , tough. |
"async \"${RUNFILES}/%s\"" % _get_runfile_path(ctx, exe.files_to_run.executable) | ||
] | ||
statements += [ | ||
"""async \"${{RUNFILES}}/{}\"""".format(_get_runfile_path(ctx, exe.files_to_run.executable)) |
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.
Is there any particular reason why you ha switched to a multiline string here (and couple other places)? Most of rules code originated from Google uses "double quoted strings"
in situations like those.
Description
Add a
kubectl
attribute to k8s_deploy, allowing to specify a customkubectl binary.
Fix #33
Related Issue
#33
Motivation and Context
Allows for easily wrapping or sandboxing the
kubectl
binary.How Has This Been Tested?
Tested locally on macOS with a k3s cluster running in Docker for Mac.
Types of changes
Checklist: