-
Notifications
You must be signed in to change notification settings - Fork 230
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
Updated RKE config machine pool schema and structure #1253
Updated RKE config machine pool schema and structure #1253
Conversation
There seems to be something wrong with the build... not sure if its my fault or not. |
@gylli251 Nah, appears to be a flaky test. I restarted 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.
Thanks for opening this PR! Looks straightforward to be and since it's an optional field, won't break existing cluster schemas. Lgtm. To answer your question - it will need to be tested by QA, please add a test template to the GH issue outlining test steps for the following cases
- Provisioning an RKE2 cluster with
api_version
set. - Upgrading the TF version of an existing RKE2 cluster to the next Terraform RC (likely 4.0.0-rc4) to make sure there are no issues / errors.
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
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.
Can you please add the new field to the docs?
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 to me. Just the comment from @a-blender outstanding:
Can you please add the new field to the docs?
@gylli251 - would you be able to add the field to the docs?
ping @gylli251 👆🏻 |
I think i added documentation that is sufficient. please let me know if more is needed. |
Dismissing this review to unblock PR
Issue:
#1249
Problem
When you are creating an elemental cluster via terraform the apiversion which rancherv2 cluster uses by default for the machine_config is wrong. You need "elemental.cattle.io/v1beta1".
Solution
Added api_version as an option for machine_config via the schema and structure code. so we can do this:
![image](https://private-user-images.githubusercontent.com/10378013/275204267-ae8a58e0-7c62-4461-9a35-35b42e90dc43.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MDQzNTYsIm5iZiI6MTczOTYwNDA1NiwicGF0aCI6Ii8xMDM3ODAxMy8yNzUyMDQyNjctYWU4YTU4ZTAtN2M2Mi00NDYxLTlhMzUtMzViNDJlOTBkYzQzLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE1VDA3MjA1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTUxMDU4OTMyNTI2MGQxMWM2Yjk4Njk0NTU5YjgxZWI4ZjAzMmZmNGJhNTI4MTIzOGQ5ZWY5M2UxOGRjM2ZkYjgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.HV-MfkJ2Mlp5nx9sHHBkmoc7MNNq9xAbtXpepfH94TM)
Testing
I have no idea how to test properly not sure if its needed since these changes are very minor.
Engineering Testing
Manual Testing
Tested the provider myself and have created/destroyed/made changes in place, more than 10 clusters without fault.
Automated Testing
QA Testing Considerations
Regressions Considerations
My first real merge request on github, please advise me if you feel like i can do stuff better!