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

[WIP] Embed example templates into clusterawsadm #617

Closed

Conversation

randomvariable
Copy link
Member

@randomvariable randomvariable commented Mar 1, 2019

What this PR does / why we need it:
Removes gettext by starting to embed the templates into clusterawsadm.

User will run the following to create their examples:

clusterawsadm alpha config init --cluster-name default --ssh-key-name default --control-plane-instance-type t3.medium <etc...>

Which will write the same outputs as before.

Also moving the examples to a top-level /examples directory for better visibility. Don't think they need to live under /cmd/clusterctl/examples

These files are used by a Bazel rule to generate a go file with the files zipped into a Go virtual file stream which is then used by the CLI to generate the examples as rendered Go templates.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #299

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

This PR will require changes to e2e, so will wait for #606 before completion.

Release note:


@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 1, 2019
@k8s-ci-robot k8s-ci-robot requested review from d-nishi and vincepri March 1, 2019 10:34
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 1, 2019
@randomvariable randomvariable changed the title wip [WIP] Embed example templates into clusterawsadm Mar 1, 2019
@k8s-ci-robot
Copy link
Contributor

@randomvariable: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-cluster-api-provider-aws-verify-boilerplate 9757cd0 link /test pull-cluster-api-provider-aws-verify-boilerplate
pull-cluster-api-provider-aws-bazel-verify 9757cd0 link /test pull-cluster-api-provider-aws-bazel-verify
pull-cluster-api-provider-aws-bazel-test 9757cd0 link /test pull-cluster-api-provider-aws-bazel-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Copy link
Contributor

@chuckha chuckha left a comment

Choose a reason for hiding this comment

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

I'm torn on the config file vs CLI args

On the one hand CLI args are easy, but on the other had I feel like this has the potential to get complex and CLI flags are not great for repeatability/infrastructure as code. Like, I can check in a configuration file but I guess checking in a command line to run isn't as easy

@randomvariable
Copy link
Member Author

Actually agree on this.

Only doing CLI flags as equivalents of the environment variables as used by envsubst in generate-yaml.sh.

What behaviour do we want to keep, and what should be jettisoned.

Don't see the point, for example of having the instance type env vars when we as cluster api devs can use kustomize patches for our workflows, and an end user should be expected to inspect and edit the cluster and machine yamls.

@randomvariable
Copy link
Member Author

How about separate commands for cluster, machine and provider components, but with the machine command, you can pass in --cluster-config cluster.yaml and it will generate machine.yamls with the same cluster name and ssh key?

@chuckha
Copy link
Contributor

chuckha commented Mar 4, 2019

so the CLI might look something like this:

clusterawsadm alpha config init cluster
clusterawsadm alpha config init machine --cluster-config cluster.yaml
clusterawsadm alpha config init provider-components

What would cluster.yaml look like?

Maybe something like?

clusterName: my-cluster
sshKeyName: my-ssh-key-name

@randomvariable
Copy link
Member Author

Was thinking of exposing the user to the real cluster object at this stage rather than present a clusterawsadm abstraction, but using the cluster object as a hint for some of the fields for new machines.

@chuckha
Copy link
Contributor

chuckha commented Mar 4, 2019

ah so, the output of clusterawsadm alpha config init cluster would be fed into clusterawsadm alpha config init machine --cluster-config cluster.yaml ?

@ashish-amarnath
Copy link
Contributor

That's how I thought this would work. This will be kinda guiding the user through the steps to help them understand what they are getting and what is all under their control.

@randomvariable
Copy link
Member Author

Yup, that's exactly it.

@vincepri
Copy link
Member

vincepri commented Mar 7, 2019

/milestone Next

Feel free to change if necessary.

@k8s-ci-robot k8s-ci-robot added this to the Next milestone Mar 7, 2019
@randomvariable
Copy link
Member Author

The underlying issue is v1alpha1

Going to finish this one before #612 .

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 5, 2019
@detiber
Copy link
Member

detiber commented Jun 5, 2019

closing, we can reconsider down the line.

/close

@k8s-ci-robot
Copy link
Contributor

@detiber: Closed this PR.

In response to this:

closing, we can reconsider down the line.

/close

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.

@randomvariable randomvariable deleted the remove-gettext branch May 26, 2020 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove requirement for GNU Make, envsubst & gettext for end-user experience
7 participants