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

40ignition-conf: support .d dir for base config fragments #676

Merged
merged 2 commits into from
Oct 30, 2020
Merged

40ignition-conf: support .d dir for base config fragments #676

merged 2 commits into from
Oct 30, 2020

Conversation

sohankunkerkar
Copy link
Member

Fixes coreos/ignition#1101
This change is required for coreos/ignition#1108
Ignition PR allows the two distros i.e. RHCOS and FCOS to carry different base.ign files with common elements and will provide some flexibility as we start to see more divergence there.

@cgwalters
Copy link
Member

I'm a bit confused, do we really need this change?
If we land coreos/ignition#1108 then we basically just need a separate overlay that only enables the afterburn-sshkeys bit from #626 in this repo that isn't inherited/included by RHCOS.

IOW for FCOS we have:

  • /usr/lib/ignition/base.d/05core.ign
  • /usr/lib/ignition/base.d/30core-afterburn-sshkeys.ign

And RHCOS just has the first or so?

@cgwalters cgwalters mentioned this pull request Oct 13, 2020
Merged
@bgilbert
Copy link
Contributor

@cgwalters Yup, I agree with your proposal.

cgwalters
cgwalters approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

LGTM!

jlebon
jlebon approved these changes Oct 21, 2020
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

One minor comment, LGTM as is!

overlay.d/15fcos/usr/lib/dracut/modules.d/fcos-ignition/module-setup.sh Outdated Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Oct 21, 2020

Is this still WIP? PR title says it is. :)

@sohankunkerkar
Copy link
Member Author

Is this still WIP? PR title says it is. :)

Actually, I wanted to get this PR in before merging this one. But I'm not sure if it's a valid concern.

@sohankunkerkar sohankunkerkar changed the title [WIP] 40ignition-conf: support .d dir for base config fragments 40ignition-conf: support .d dir for base config fragments Oct 22, 2020
@jlebon jlebon added the hold label Oct 22, 2020
@jlebon
Copy link
Member

jlebon commented Oct 22, 2020

Oh right, we need to hold this until the Ignition support is there. Otherwise we'd essentially be reverting #626.

Added a hold label.

15fcos: copy all base config fragments to base.d dir

Hmm, is this accurate? We're just splicing out a part of the base config into a base.d, right?

This change is required for coreos/ignition#1101

How something like:

Make use of the new support for config fragments in Ignition (coreos/ignition#1108) to move out the Afterburn bit added in #626 to a separate overlay directory so that RHCOS can more easily avoid it.

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Oct 22, 2020 •
edited
Loading

15fcos: copy all base config fragments to base.d dir

Hmm, is this accurate? We're just splicing out a part of the base config into a base.d, right?

@jlebon Yeah, you're right. I think I missed that point to change the commit message and the commit body message after my last changes.

This change is required for coreos/ignition#1101

How something like:

Make use of the new support for config fragments in Ignition (coreos/ignition#1108) to move out the Afterburn bit added in #626 to a separate overlay directory so that RHCOS can more easily avoid it.

+1

bgilbert
bgilbert reviewed Oct 23, 2020
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

Dracut module directory names usually start with a two-digit sequence number.

Remember to move and update the README.md from 40ignition-conf.

overlay.d/15fcos/usr/lib/dracut/modules.d/fcos-ignition/module-setup.sh Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/lib/dracut/modules.d/fcos-ignition/module-setup.sh Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/lib/dracut/modules.d/fcos-ignition/module-setup.sh Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/lib/dracut/modules.d/fcos-ignition/module-setup.sh Outdated Show resolved Hide resolved
bgilbert
bgilbert reviewed Oct 23, 2020
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

10fcos-ignition will run before 40ignition-conf, which is currently harmless but a little confusing. Maybe call it 50ignition-conf-fcos?

overlay.d/15fcos/usr/lib/dracut/modules.d/10fcos-ignition/README.md Outdated Show resolved Hide resolved
bgilbert
bgilbert approved these changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

The core config has a dash after the sequence number in the filename, but the Afterburn fragment doesn't. LGTM otherwise.

@sohankunkerkar sohankunkerkar added hold and removed hold labels Oct 29, 2020
@sohankunkerkar sohankunkerkar mentioned this pull request Oct 29, 2020
Merged
@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Oct 29, 2020 •
edited
Loading

After having some discussion with @bgilbert and @arithx, it appears to me that this PR can't be merged in its current form since it requires coreos/ignition#1108, which leads to cutting another Ignition release. I think we already released a new version of Ignition a few days ago, and we wouldn't want to rush to create another release. Without the new Ignition release, this PR will certainly break CI.
In order to avoid CI breakage, we've decided to drop the base.ign removal from this PR and split it out to a separate PR (#717).

@sohankunkerkar
40ignition-conf: add new base config
e8402f8 
Ignition now supports a `base.d` directory for base config fragments that allows FCOS and
RHCOS to maintain the same `00-core.ign`, and facilitate the distro-specific changes by placing
the other base config fragments in a separate `overlay` directory(coreos/ignition#1108).
This commit places 00-core.ign in a `base.d` directory.

Also, keeping the legacy base config and the Afterburn bit as-is, otherwise, it will break CI.
Once we have the new Ignition release, this PR (#717) will remove the legacy base config and revert #626
@sohankunkerkar
15fcos: place afterburn-sshkeys.ign in 50ignition-conf-fcos
96a2489 
Make use of the new support for config fragments in Ignition (coreos/ignition#1108)
to move out the Afterburn bit added in #626
to a separate overlay directory so that RHCOS can more easily avoid it.
@sohankunkerkar sohankunkerkar merged commit b607e44 into coreos:testing-devel Oct 30, 2020
@sohankunkerkar sohankunkerkar deleted the config-dir branch October 30, 2020 02:57
c4rt0 pushed a commit to c4rt0/fedora-coreos-config that referenced this pull request Mar 27, 2023
@openshift-merge-robot
Merge pull request coreos#676 from miabbott/use_buildfetch_ci
1b9ee3d 
ci: enable use of buildfetch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlebon jlebon jlebon approved these changes

@lucab lucab lucab left review comments

@arithx arithx arithx approved these changes

@bgilbert bgilbert bgilbert approved these changes

@cgwalters cgwalters cgwalters approved these changes

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

Successfully merging this pull request may close these issues.

Consider supporting .d directory for base config fragments
6 participants
@sohankunkerkar @cgwalters @bgilbert @jlebon @lucab @arithx