-
Notifications
You must be signed in to change notification settings - Fork 246
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
Stablize Ignition spec 3.5 #1922
Conversation
@madhu-pillai Thank you for doing this; Would you mind updating this checklist #1925 with the steps completed? I dont think I could do a stablization without having a checkbox lol too many steps. |
HI @prestist , |
@madhu-pillai awesome, so note; there are external packages that need to be updated after this lands and then additionally we need to stabilize ignition in butane | ignition-config-rs | ign-converter |. see #1553 (comment) |
Ok @madhu-pillai I have reviewed your changes, it's a bit intense in one commit. Is there any way I could ask you to follow a similar structure to this https://github.com/coreos/ignition/pull/1558/commits To my eye; I dont see anything wrong with your PR however due to the sensitive nature I want to make sure before approving. Sorry for the difficult ask but its not very consumable atm. |
Will do that... |
c9ce1c5
to
bed9df1
Compare
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.
Gennerally lgtm, small changes and a question;
Are we wanting CEX to be considered stable? @travier
config/doc/ignition.yaml
Outdated
@@ -384,7 +384,7 @@ root: | |||
desc: describes the IBM Crypto Express (CEX) card configuration for the luks device. | |||
children: | |||
- name: enabled | |||
desc: whether or not to use a CEX secure key to encrypt the luks device. |
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.
We should not make this change as a part of the pr, it makes it confusing. this can be fixed by the already opened external pr.
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.
Done
bed9df1
to
045b680
Compare
Hi, The moment before the push, the test was success. not sure why it failed on the go part. |
You might need to rebase- golang 1.23 is having issues with the ./build script and there was a changed committed to main to fix it. |
045b680
to
a207916
Compare
Hi, I did rebase before the push and it is upto date.
|
@madhu-pillai it looks like unit-tests are failing; are you saying they are passing locally for you? |
Hi @prestist , Yes, the test pass locally.
|
Yes, this is why we need this spec stabilization. |
It's hard to review this as is as you need to strictly follow the steps and create intermediary commits to make the changes visible. I'm re-splitting the changes. See: #1558 We might also have to look at if we need to re-add: |
a207916
to
fe10ff7
Compare
Let's see if this passes tests. I'll have to review it again. |
The test got passed when i push the PR as single commit as well as later i split the PR. Then i push the PR after the correction in |
c2758dc
to
9721e97
Compare
9721e97
to
df18924
Compare
Thanks for fixing the CI! Rebased |
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! 🚀
Wow thank you both very much, this looks good to my eyes, the commit structure gives me much more confidence to verify this is correct, I am sure it was painful but thank you for making it happen.
Hi, Is there anything pending from my side to progress on the new feature? |
If you look at issue here => #1925 We will have to make the changes to some other packages once this is released. Yikes wrong button sorry. |
https://issues.redhat.com/browse/COS-2870
I've completed the changes in ignition to stablize 3.5.0 from the issue template till
Update Docs
https://github.com/coreos/ignition/blob/main/.github/ISSUE_TEMPLATE/stabilize-checklist.md .Kindly review and suggest for any further changes.
Fixes: #1925