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

📖 E2e: Document self hosted machine templates #9613

Merged
merged 1 commit into from
Oct 26, 2023
Merged

📖 E2e: Document self hosted machine templates #9613

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions test/e2e/self_hosted.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ type SelfHostedSpecInput struct {
// If true, the variable KUBERNETES_VERSION is expected to be set.
// If false, the variables KUBERNETES_VERSION_UPGRADE_FROM, KUBERNETES_VERSION_UPGRADE_TO,
// ETCD_VERSION_UPGRADE_TO and COREDNS_VERSION_UPGRADE_TO are expected to be set.
// 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
Comment on lines +62 to 66
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙁

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@sbueringer sbueringer Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@sbueringer sbueringer Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me


// ControlPlaneMachineCount is used in `config cluster` to configure the count of the control plane machines used in the test.
Expand Down