Skip to content
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.

Sign up for GitHub

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

Jump to bottom

new butane variant: fedora-iot #487

Merged
merged 1 commit into from
Dec 4, 2023
Merged

new butane variant: fedora-iot #487

merged 1 commit into from
Dec 4, 2023

Conversation

say-paul
Copy link
Contributor

@say-paul say-paul commented Sep 6, 2023 •
edited
Loading

using the r4e config with fiot wrapper
Docs added to include the new variant

@say-paul say-paul force-pushed the butane_for_fedora-iot branch 2 times, most recently from fad3e8a to b13dc89 Compare September 6, 2023 13:58
@travier
Copy link
Member

travier commented Sep 6, 2023

I think you should add only the latest version as 1.0 and the experimental one as 1.1_exp.

@travier
Copy link
Member

travier commented Sep 6, 2023

Why are you removing / renaming the R4E ones? You can not remove a config version once it's been released.

@say-paul
Copy link
Contributor Author

say-paul commented Sep 6, 2023 •
edited
Loading

Why are you removing / renaming the R4E ones? You can not remove a config version once it's been released.

Since we want butane to support both r4e and fedora-iot, so renaming the config path only for developer understanding. The end functionality and the variant names remains same. Do you see any other issue?

I think you should add only the latest version as 1.0 and the experimental one as 1.1_exp.

This is to make r4e and fiot in sync, so fiot_v1.0 being same as r4e_v1.1 f can create confusion and maintenance overhead. same for fiot_v1.1_exp -> r4e_v1.2_exp. Thoughts?

@travier
Copy link
Member

travier commented Sep 6, 2023

If it's the same thing, why duplicate everything?

@prestist
Copy link
Contributor

prestist commented Sep 6, 2023

@say-paul A bit confused. As @travier said, we cannot remove an already released config, it would break anyone using it. Additionally, it sounds like the 'fiot' would be independent of rfe? while currently they have the same needs is there no eventuality where they fork to some degree?

@say-paul
Copy link
Contributor Author

say-paul commented Sep 7, 2023

If it's the same thing, why duplicate everything?

@travier So idea is both rfe and fedora-iot user can use butane with their respective os variant. Since r4e is already there, this PR would simply add a fiot variant name with minimal code duplication.

As @travier said, we cannot remove an already released config, it would break anyone using it.

RegisterTranslator("r4e", "1.0.0", iot1_0.ToIgn3_3Bytes)
RegisterTranslator("r4e", "1.1.0", iot1_1.ToIgn3_4Bytes)
RegisterTranslator("r4e", "1.2.0-experimental", iot1_2_exp.ToIgn3_5Bytes)
RegisterTranslator("fiot", "1.0.0", iot1_0.ToIgn3_3Bytes)
RegisterTranslator("fiot", "1.1.0", iot1_1.ToIgn3_4Bytes)
RegisterTranslator("fiot", "1.2.0-experimental", iot1_2_exp.ToIgn3_5Bytes)

I understand this controls the variant name in the butane file which didn't change.

@travier @prestist Can you please clarify how it will break for existing user?

@travier
Copy link
Member

travier commented Sep 7, 2023

It feels to me that R4E variants should use version numbers based on RHEL releases where the first version config started being supported while the IoT ones should keep the 1.x version numbers as we don't have a strong "no-rebase" policy in Fedora and we land new Ignition spec support there as we go.

@travier
Copy link
Member

travier commented Sep 7, 2023

@travier So idea is both rfe and fedora-iot user can use butane with their respective os variant. Since r4e is already there, this PR would simply add a fiot variant name with minimal code duplication.

The lifecycle for RHEL 4 Edge and Fedora IoT do not match and the Ignition spec versions supported for a given Fedora IoT release tells nothing for RHEL 4 Edge and vice versa, so having the same numbers will not get us much and might instead create confusion.

@say-paul
Copy link
Contributor Author

say-paul commented Sep 7, 2023

The lifecycle for RHEL 4 Edge and Fedora IoT do not match and the Ignition spec versions supported for a given Fedora IoT release tells nothing for RHEL 4 Edge and vice versa, so having the same numbers will not get us much and might instead create confusion.

Agreed, I will modify it accordingly.

@travier
Copy link
Member

travier commented Sep 11, 2023

I've made #489 to track the R4E change.

@say-paul
Copy link
Contributor Author

@travier made the changes as per the discussion.

7flying
7flying reviewed Sep 11, 2023
View reviewed changes
docs/config-fiot-v1_0.md Outdated Show resolved Hide resolved
@travier
Copy link
Member

travier commented Sep 11, 2023 •
edited
Loading

This is missing all some docs update. The table has to be updated, etc.

@say-paul
Copy link
Contributor Author

say-paul commented Sep 11, 2023 •
edited
Loading

This is missing all some docs update. The table has to be updated, etc.
Thanks for finding that out.

I think you are referring to the spec docs, updated them.

travier
travier reviewed Sep 11, 2023
View reviewed changes
docs/specs.md Outdated Show resolved Hide resolved
@say-paul say-paul force-pushed the butane_for_fedora-iot branch 2 times, most recently from eb576de to 9a45a28 Compare September 11, 2023 15:50
miabbott
miabbott approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@miabbott miabbott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@miabbott
Copy link
Member

miabbott commented Oct 3, 2023

@say-paul Are the CI errors still valid? Looks like they are just gofmt failures

@say-paul say-paul force-pushed the butane_for_fedora-iot branch 2 times, most recently from 590c508 to 1606137 Compare October 3, 2023 13:15
@cgwalters
Copy link
Member

Is there actually any "high level" sugar at all today in the r4e/fedora-iot versions?

There's only a little bit for fedora-coreos around things like boot raid etc.

I guess today we're disallowing partitioning in iot/edge?

I wonder if it'd make sense to introduce a generic e.g. "generic-systemd" style variant that just is YAML sugar for writing files and systemd units basically...

prestist
prestist approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@prestist prestist left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this has been sitting for so long, and thank you for going through the steps to add a new variant. I do have a small nit on your commit message. I would prefer it be more like =>
Add new fiot variant for fedora-iot

Otherwise it lgtm

@say-paul
Copy link
Contributor Author

say-paul commented Nov 1, 2023

@prestist done.

@miabbott
Copy link
Member

miabbott commented Nov 7, 2023

@prestist @travier Anything left to do in order to merge? I think all comments have been addressed at this point.

@prestist
Copy link
Contributor

@say-paul @miabbott not from my perspective. @travier anything else ?

@prestist
Copy link
Contributor

prestist commented Dec 4, 2023

@say-paul

To get past the ci "Test 1.21.x ..." you need to rebase and -f push. The ci was changed external to this pr.

Otherwise from what I see this LGTM still.

@say-paul
Add new fiot variant for fedora-iot
fed77f6 
v1_0 is compatible with ignition 3.4.0
v1_1_exp is experimental version compatible
with ignition 3.5.0
docs added for fiot

Signed-off-by: Sayan Paul <saypaul@redhat.com>
@say-paul
Copy link
Contributor Author

say-paul commented Dec 4, 2023

@prestist done

@prestist
Copy link
Contributor

prestist commented Dec 4, 2023

After speaking with @camgranella I am going to merge this as it lgtm!

@prestist prestist merged commit 25265e5 into coreos:main Dec 4, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7flying 7flying 7flying left review comments

@miabbott miabbott miabbott approved these changes

@prestist prestist prestist approved these changes

@travier travier Awaiting requested review from travier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@say-paul @travier @prestist @miabbott @cgwalters @7flying