Updates for usability of GKE self managed setup script #759
Conversation
|
Hi @KatrinaHoffert. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
And yes, my bash skills are not that great. There's probably tons of room for micro-optimizations and all with the tools I use for filtering text and all. I tried to use There's some areas for future improvement that I did not do due to time constraints. Future improvements could be:
|
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
- Removed arguments that no longer exist and those that are defaults. - Updated command to not depend on sh (not available by default). Use same pattern as most GKE containers do. - Image URL now ensures that the YAML would be invalid (more obvious if you miss it).
All values are overridable in case the generation messes up.
We build and push automatically if the user doesn't provide --image-url.
Better UX this way, too. Also more output about progress and consistency in file names.
Also fixed details on what the script does. It was confusing before and had some order wrong.
1. Restored confirmation for cluster being ready after disabling the default GLBC. Also made this affected by `--no-confirm`, since the intent of that was to allow non-interactive usage (cause forced interactive scripts are gross). Made that set `--quiet` for gcloud commands while I was at it, since some of those can confirm (and made it a versatile "provide arbitrary extra gcloud flags" thing). Only applied to mutating gcloud commands. 2. Build and push no longer implicit. As a result, either the `--image-url` or `--build-and-push` must be set. This is now validated. 3. Added validation for if CONTAINER_PREFIX is set while I was at it, since it can break build-and-push. I doubt anyone sets it, but better safe than sorry. This just avoids an inevitible error. 4. Placeholder used in GLBC YAML changed. 5. Using `.gen` as extension for generated files. 6. Make command is now logged so user sees what we're doing. 7. Clarified help text.
This is very useful for testing. It uses kubectl --dry-run where possible, so we're actually testing the structure of the YAML files (the creation of the key does not use this since it depends on running a real command). This command can also be used to quickly and cleanly get the commands this script would run for manual usage. Fixed bug with --cleanup where it would ignore any args after the --cleanup tag (which could result in running the wrong things!).
Also fixed dumb quote bug that wouldn't play nice with eval.
|
All done, @rramkumar1 |
|
/lgtm Thanks for all these changes! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KatrinaHoffert, rramkumar1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
For issue #758.
This does several things to make this setup much easier. But in short, it allows a single command to be used to setup for the vast majority of cases (with just
./gke-self-managed.sh -n CLUSTER_NAME -z ZONE).glbc.yamlfile is automatically setup with the image instead of letting users create an invalid deployment that won't be caught till runtime.make pushdoes.Opted not to make a test for it this ticket. We really should have some kinda test (since the script got terribly broken in 2 ways), but it's quite difficult to test given that it requires cluster ownership, needs push access to some container registry, requires an existing cluster, and is very slow no matter how you cut it (it needs to restart a cluster and realistically, a test would also need to create an ingress, which takes a few minutes). I'll make a new issue for testing it, but it's likely to be lower priority as the ROI is complicated.
This is a non-breaking change, since the existing
gce.conffile is used as a template and if it were already filled in, those fields would just be ignored. The files that need modification are now copied to and.gitignored, too, so no risks of accidental commits or annoyance of having them show up in thegit status.