Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

dracut: adapt for s/oem/platform in Ignition #45

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Feb 26, 2019

With coreos/ignition#735 merged, we need to fix
our service units to use --platform here. Also just do the equivalent
thing here and start to move away from the oem terminology. Crucially
though, we still look for coreos.oem.id until downstreams have
adapted (notably coreos-assembler).

Copy link
Contributor

@arithx arithx left a comment

Choose a reason for hiding this comment

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

LGTM

dracut/30ignition/ignition-generator Outdated Show resolved Hide resolved
@ajeddeloh
Copy link
Contributor

We're not building ignition rpms from master yet, so we ought to merge this to a different branch instead of master. I haven't gotten around to setting up a copr for the 3.0.0 work yet, but that seems like what we ought to do while we develop 3.0.0.

@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2019

We're not building ignition rpms from master yet, so we ought to merge this to a different branch instead of master.

Wouldn't it be cleaner to just match the Ignition repo and use a spec2x branch here too?

@ajeddeloh
Copy link
Contributor

Yeah that's probably better. Since the RPM spec file refers to it by commit that should be fine.

@ajeddeloh
Copy link
Contributor

But we should create that branch and add notes to the readmes before merging this

@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2019

OK, in favour of splitting. Though for this patch, I'd rather we maintain compatiblity for coreos.oem.id regardless until we can fully abandon the spec2x branches, and permanently move coreos-assembler to coreos.platform.id.

@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2019

and permanently move coreos-assembler to coreos.platform.id

Actually, we can take this opportunity to fix #31 as well and have the new name be e.g. ignition.platform.id.

Though we'll want to be careful with other consumers as @lucab mentioned.

With coreos/ignition#735 merged, we need to fix
our service units to use `--platform` here. Also just do the equivalent
thing here and start to move away from the oem terminology. Crucially
though, we still look for `coreos.oem.id` until downstreams have
adapted (notably coreos-assembler).

Closes: coreos#31
@dustymabe
Copy link
Member

this LGTM - re: spec2x I'd honestly rather just keep a single branch here and move to 3.0 spec sooner than later. For RHCOS maybe we could just backport patches in the rpm?

@dustymabe
Copy link
Member

For RHCOS maybe we could just backport patches in the rpm?

only for ones that are absolutely required, of course

@jlebon jlebon changed the title WIP: dracut: adapt for s/oem/platform in Ignition dracut: adapt for s/oem/platform in Ignition Feb 26, 2019
@jlebon
Copy link
Member Author

jlebon commented Feb 26, 2019

OK, this is tested now, both using the old and the new kernel args. Dropping WIP, though let's see what people think re. ignition.platform.id.

@dustymabe
Copy link
Member

though let's see what people think re. ignition.platform.id.

+1 for ignition.platform.id

@dustymabe
Copy link
Member

this LGTM - re: spec2x I'd honestly rather just keep a single branch here and move to 3.0 spec sooner than later. For RHCOS maybe we could just backport patches in the rpm?

discussed in IRC. We'll go with this for now:

ignition{,-dracut} - master -> Fedora rpms
ignition{,-dracut} - spec2x -> EL7 rpms

@dustymabe
Copy link
Member

FYI: created the spec2x branch: https://github.com/coreos/ignition-dracut/tree/spec2x

@dustymabe dustymabe merged commit 9aac3c5 into coreos:master Feb 27, 2019
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.

ignition.platform.id makes sense in the context of this repo, which is intended to be distro-independent. But for FCOS, at least, we'll be configuring a number of other distro behaviors (whether to start a platform agent, etc.) on the "Ignition" platform ID. That might make the name confusing. I don't have a better suggestion, though.

@@ -95,4 +98,4 @@ if [[ $(systemd-detect-virt || true) =~ ^(kvm|qemu)$ ]]; then
oem_id=qemu
fi

echo "OEM_ID=$(cmdline_arg coreos.oem.id ${oem_id})" > /run/ignition.env
echo "PLATFORM_ID=$platform_id" > /run/ignition.env
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the oem_id logic in the previous stanza.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Added a fix for this in #46.

jlebon added a commit to jlebon/ignition-dracut that referenced this pull request Feb 28, 2019
@jlebon jlebon deleted the pr/oem-platform branch March 1, 2019 21:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants