-
Notifications
You must be signed in to change notification settings - Fork 15
fix: validation for new default ocp versions #870
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
Conversation
vkuma17
left a 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.
looks good
|
/run pipeline |
|
just noticed that as per this logic 4.14 with rhcos will still work here which is not supported but since 4.14 is returned as a valid version from the list final contains condition will evaluate to true |
vkuma17
left a 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.
Need some logic update, current logic will completely ignore OS checks
vkuma17
left a 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.
LGTM
|
/run pipeline |
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.
@aamadeuss one more small change is required. right now for version 4.20 and higher we only validate if its a valid version and no validations on OS.
starting 4.21 onwards, only RHCOS will be supported and in 4.20 RHCOS and RHEL 9 will both be supported, so for now can we just add another condition just for 4.20 similar to 4.19 and another condition checking all versions >= 4.21 and OS = "RHCOS"
|
/run pipeline |
|
Retrying pipeline as it seems to be working now |
|
/run pipeline |
|
/run pipeline |
|
Pipeline failed because of schematics test, retrying |
|
/run pipeline |
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 does not really achieve what we want. Your still hard coding versions here (you just added 4.20 and 4.21 which don't even exist yet). The goal here is to see if we can have logic that does not require hard coding versions. Alternatively, I suggested that we actually test what happens if we remove the validation. Will the provider catch unsupported OS if tried? Does it fail at plan or apply?
Maybe if we know an OS is longer supports after a certain version, we check for that. For example we know RHEL8 does not work after a certain version. Lets validate that. And then as Operating system versions are deprecated in later versions, we add validation for those. But what we don’t want is validation failing on new versions for operating systems that are supported. Like what happened when we moved to 4.19
@ocofaigh i agree that we are still hardcoding versions but this PR is future proof... we hardcoded 4.20 because, 4.20 is going to support both RHCOS and RHEL9 while from 4.21 onwards only RHCOS will be supported. With this change, we don't need to make any changes afterwards even with 4.22 and onward versions |
|
🎉 This PR is included in version 3.73.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Description
Fixes ocp version validation for
worker_poolsso it does not fail for a newly released default version.Added a data block look up to fetch the currently supported OCP versions so that versions added in the future will not need to be added separately in the validation.
Fixes #849
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers