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
WINC-607: [e2e] Modularize BYOH configuration test #865
WINC-607: [e2e] Modularize BYOH configuration test #865
Conversation
Skipping CI for Draft Pull Request. |
/test lint |
/test unit |
/test azure-e2e-operator |
test/e2e/create_test.go
Outdated
} | ||
|
||
// configureBYOHWithMachineSets configures BYOH instances using MachineSets | ||
func (tc *testContext) configureBYOHWithMachineSets() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe rename this to something like newBYOHConfigMapFromMachineSet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about that, do you anticipate newBYOHConfigMapFromMachineSet
to return a reference of the windows-instances
ConfigMap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm just thinking that configureBYOHWithMachineSets
doesn't really capture what the function is doing. The configuration of BYOH instances will be done by WMCO, this just enables it.
Alternatively maybe createBYOHConfigMapFromMachineSet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Updated to provisionBYOHConfigMapWithMachineSet
in 3f4f800 .
This function provisions the given number of BYOH instances and creates the windows-instances
ConfigMap with the instances information.
PTAL
test/e2e/create_test.go
Outdated
}) | ||
} | ||
|
||
// configureBYOHWithMachineSets configures BYOH instances using MachineSets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function will prevent Machine CSRs from being approved in the future, right?
Lets make sure thats captured in the function comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 26df7d8
26df7d8
to
3f4f800
Compare
/test azure-e2e-operator |
test/e2e/create_test.go
Outdated
return tc.createWindowsInstanceConfigMap(machines) | ||
} | ||
|
||
// disableClusterMachineApprover disables the Cluster Machine Approver, by scaling down Cluster Version Operator and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM though this will probably need to be rebased once #873 merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
This commit modularizes the testBYOHConfiguration test, so that it is easy to maintain and can be extended. This is a pre-work to introduce support for platform=none.
3f4f800
to
a0a2243
Compare
Rebased with #873 |
/test azure-e2e-operator |
/test lint |
/test unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Depends on #909 |
Depends on #910 |
/cherry-pick community-4.9 #908 has merge |
@jrvaldes: #865 failed to apply on top of branch "community-4.9":
In response to this:
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. |
/cherry-pick community-4.9 #908 has merge |
@jrvaldes: #865 failed to apply on top of branch "community-4.9":
In response to this:
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. |
/cherry-pick release-4.8 |
@mansikulkarni96: #865 failed to apply on top of branch "release-4.8":
In response to this:
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. |
/cherry-pick release-4.9 |
@mansikulkarni96: #865 failed to apply on top of branch "release-4.9":
In response to this:
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. |
/cherry-pick release-4.10 |
@mansikulkarni96: #865 failed to apply on top of branch "release-4.10":
In response to this:
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. |
/cherry-pick community-4.9 |
@mansikulkarni96: #865 failed to apply on top of branch "community-4.9":
In response to this:
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. |
/cherry-pick release-4.9 |
@mansikulkarni96: new pull request created: #950 In response to this:
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. |
/cherry-pick community-4.9 |
@mansikulkarni96: new pull request created: #951 In response to this:
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. |
@mansikulkarni96: #865 failed to apply on top of branch "release-4.8":
In response to this:
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. |
/cherry-pick community-4.8 |
@mansikulkarni96: new pull request created: #967 In response to this:
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. |
/cherry-pick release-4.8 |
@mansikulkarni96: new pull request created: #969 In response to this:
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. |
This commit modularizes the testBYOHConfiguration test, so that
it is easy to maintain and can be extended. This is a pre-work
to introduce support for platform=none.