-
Notifications
You must be signed in to change notification settings - Fork 245
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
base: main
Are you sure you want to change the base?
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
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package v3_6_experimental |
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.
Commit name is wrong; should be
config: copy v3_5 to v3_6_experimental
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
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software |
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.
Im not sure why but because stabilizing and renaming happened at the same time there seems to be a very large diff. It makes everything look like new content (not a simple rename) i.e look here 86d17f1
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.
Ah, Its because when looking at it commit by commit the removal of the files was at a separate point.
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.
Is it because i did the mv without git?
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 |
fe10ff7
to
c2758dc
Compare
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