-
Notifications
You must be signed in to change notification settings - Fork 223
OCPBUGS-71236: Align featureGate validations with openshift #6138
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
base: main
Are you sure you want to change the base?
Conversation
|
@copejon: This pull request references Jira Issue OCPBUGS-71236, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: copejon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
TODO: I need to update rf featuregate tests |
f83244c to
96c9356
Compare
|
it looks good to me |
Makefile
Outdated
|
|
||
| # Set variables for test-unit target | ||
| GO_TEST_FLAGS=$(GO_BUILD_FLAGS) | ||
| GO_TEST_ARGS=$(GO_LD_FLAGS) |
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.
I can't find it being used anywhere either in our code or deps/ or vendor/. Why do we need it?
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 is to enable unit testing featuregate prerun functions that handle version skew validation for upgrade/downgrades.
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.
I'm glad you pointed this out. Relying on the buildflag version values wasn't entirely in line with how the unit tests were checking the validation functions. The cleaner solution was to mock the function that validation used to get the compiled version values and override it in tests to manipulate the version values as needed.
|
/jira refresh |
|
@copejon: This pull request references Jira Issue OCPBUGS-71236, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
removed: RF tests related to version detection. these are covered sufficiently by unit tests
4d4f5b6 to
a27e240
Compare
Added getExecutableVersion function variable to allow mocking the executable version in tests (the version is normally set via ldflags at compile time, making it impossible to test upgrade/downgrade scenarios without mocking) Updated TestFeatureGateLockManagement_FirstRun, TestFeatureGateLockManagement_ConfigChange, and TestFeatureGateLockManagement_VersionChange to mock the version instead of relying on ldflags Removed GO_TEST_ARGS from Makefile since ldflags are no longer required for unit tests
|
/cherrypick release-4.21 |
|
@copejon: once the present PR merges, I will cherry-pick it on top of DetailsIn 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-sigs/prow repository. |
| { | ||
| name: "feature-gates-custom-no-upgrade-with-feature-set-empty", | ||
| config: func() *Config { | ||
| c := mkDefaultConfig() | ||
| c.ApiServer.FeatureGates.FeatureSet = "" | ||
| c.ApiServer.FeatureGates.CustomNoUpgrade.Enabled = []string{"feature1"} | ||
| c.ApiServer.FeatureGates.CustomNoUpgrade.Disabled = []string{"feature2"} | ||
| return c | ||
| }(), | ||
| expectErr: true, | ||
| }, |
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.
because you are removing this test, I understand the following configuration is a valid one, right? If so, I'd prefer to change the expectErr: to false
apiServer:
featureGates:
featureSet: ""
customNoUpgrade:
enabled:
- "CPUManagerPolicyBetaOptions"
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.
Right, the case is not validated. I'll re-add and set expectErr to false.
| c.ApiServer.FeatureGates.CustomNoUpgrade.Disabled = []string{"feature2"} | ||
| return c | ||
| }(), | ||
| expectErr: true, |
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.
Wouldn't be better to change the expectErr instead of removing the whole test?
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.
Will revert and flip expectErr
… typo in feature-gates.robot
|
/verified by copejon,ci |
|
@copejon: This PR has been marked as verified by DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@copejon: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
No description provided.