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.

Sign up for GitHub

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

Jump to bottom

Get rid of the clusterctl upgrade test fields exposed as environment variables #8535

Closed
furkatgofurov7 opened this issue Apr 17, 2023 · 5 comments · Fixed by #10609
Closed

Get rid of the clusterctl upgrade test fields exposed as environment variables #8535

furkatgofurov7 opened this issue Apr 17, 2023 · 5 comments · Fixed by #10609
Assignees
@sbueringer
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@furkatgofurov7
Copy link
Member

What steps did you take and what happened?

Currently, clusterctl upgrade tests exposes multiple fields from ClusterctlUpgradeSpecInput as environment variable. Those env vars are only used by other infra providers and they can always read an env var in their _test.go file if they want to do that.

What did you expect to happen?

Not to expose the fields as env vars. Please see #8518 (comment) for more info

Cluster API version

main and supported release branches

Kubernetes version

No response

Anything else you would like to add?

No response

Label(s) to be applied

/kind bug
One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels.
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 17, 2023
@furkatgofurov7
Copy link
Member Author

/cc @sbueringer feel free to edit as much as needed if I missed something here, thanks!

@furkatgofurov7 furkatgofurov7 mentioned this issue Apr 17, 2023
Merged
@killianmuldoon
Copy link
Contributor

/triage accepted

Sounds like a good cleanup - a good first step would be to scan the public CAPI-based provider e2e tests to see if any of these are being used as environmental variables today.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 17, 2023
@sbueringer
Copy link
Member

I think they are used today somewhere, but I think this shouldn't necessarily block us from removing them as there's an alternative (reading them in _test.go).

I just don't think it's sustainable for us to expose all "version-specific" fields as environment variables in addition to exposing them as fields. (also it's not ideal to have a Go API that additionally has env vars as an implicit API).

For example we recently added WorkloadKubernetesVersion which would also have to expose as env var to make it work. There might be more fields that we already have that we would have to expose to make the various permutations configurable via env vars (looking at our clusterctl upgrade _test.go)

@fabriziopandini
Copy link
Member

/priority backlog

@k8s-ci-robot k8s-ci-robot added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Apr 11, 2024
@fabriziopandini
Copy link
Member

/assign @sbueringer

@sbueringer sbueringer mentioned this issue May 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbueringer sbueringer

Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🌱 Drop support for INIT env variables in clusterctl upgrade test sbueringer/cluster-api
5 participants
@sbueringer @fabriziopandini @k8s-ci-robot @furkatgofurov7 @killianmuldoon