馃摉 E2e: Document self hosted machine templates #9613
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
do-not-merge/needs-area
k8s-ci-robot
added
area/e2e-testing
The self hosted test has optional variables that can be used to switch the infrastructure machine template when doing the upgrade. This documents the variables and also explains that the templates will need to be labeled in order to move with the cluster.
lentzi90
force-pushed
the
lentzi90/document-self-hosted-machine-templates
branch
from
3a598fc
to
24bb4ba
Compare
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: ab5dc183378c4607bc200f6c69dae3f1bbd90a6c
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: killianmuldoon 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
k8s-ci-robot
added
the
do-not-merge/hold
| // There are also (optional) variables CONTROL_PLANE_MACHINE_TEMPLATE_UPGRADE_TO and | ||
| // WORKERS_MACHINE_TEMPLATE_UPGRADE_TO to change the infrastructure machine template | ||
| // during the upgrade. Note that these templates need to have the clusterctl.cluster.x-k8s.io/move | ||
| // label in order to be moved to the self hosted cluster (since they are not part of the owner chain). | ||
| SkipUpgrade bool |
There was a problem hiding this comment.
Sorry for the last minute change!
Could you move the new block up to L43 i.e. before any of these variables and godocs are defined. This comment doesn't make sense in this block IMO.
There was a problem hiding this comment.
Hmm you mean it would be for the E2EConfig? I see how it is relevant to it, but I wonder if people will find it there. 馃
All these variables that are explained here are related to SkipUpgrade since they are for the upgrade, so from that perspective it makes sense to find them explained here. The e2e config on the other hand is a common input to all tests so I think I would easily miss any comments around that 馃檨
There was a problem hiding this comment.
I mean to put these in a seperate block inside SelfHostedSpecInput defining the environmental variables that can be set for this test bu aren't part of the go type.
We can add them above or after SkipUpgrade if you think that makes them easier to find for users, but having them as part of the godoc on this field feels wrong.
There was a problem hiding this comment.
What do you mean in a separate block? If the godoc is not directly above a field or a struct it won't show up in the rendered godoc afaik
There was a problem hiding this comment.
For me it makes sense to document these variables here, like we did for KUBERNETES_VERSION_UPGRADE_FROM and friends.
Basically these are the variables relevant for the upgrade case
(I think if we don't want them here we should move all of them up to the struct)
|
/lgtm |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
I'm unsure if this should be 馃摉 or 馃尡. Please change as needed.
What this PR does / why we need it:
The self hosted test has optional variables that can be used to switch the infrastructure machine template when doing the upgrade. This documents the variables and also explains that the templates will need to be labeled in order to move with the cluster.
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 #