coreos-assembler icon indicating copy to clipboard operation
coreos-assembler copied to clipboard

Move variant setup from `cosa init` to `cosa build`

Open travier opened this issue 2 years ago • 10 comments
trafficstars

Feature Request

Currently, the variant to build is specified at cosa init time and then stored in src/config.json and then read by all other cosa commands: https://github.com/coreos/coreos-assembler/blob/main/src/cmdlib.sh#L177

The variant is also stored in the meta.json after a cosa build but this is apparently not working right now for an uknown reason: https://github.com/coreos/coreos-assembler/blob/main/src/cmd-build#L467

As suggested by @jlebon, we could remove the duplication here by passing the variant as an argument to cosa build only instead and have all later steps & kola read the variant value from the meta.json value.

See:

  • https://github.com/coreos/fedora-coreos-pipeline/pull/787
  • https://github.com/coreos/coreos-assembler/pull/3274
  • https://github.com/coreos/coreos-assembler/pull/2934

We'll also have to change RHCOS CI to adapt to it: https://github.com/openshift/os/blob/master/ci/prow-entrypoint.sh As well as the pipeline: https://github.com/coreos/fedora-coreos-pipeline

Proposed UX:

cosa init
cosa fetch --variant="rhel-9.0"
cosa build --variant="rhel-9.0"

travier avatar Dec 13 '22 17:12 travier

So, it looks like we'd have to write the meta.json file starting with the fetch command and load it for the build command if we don't want to have to specify the variant two times.

travier avatar Dec 13 '22 17:12 travier

And reading the bump-lockfile job, it looks like we'll need that for the buildfetch command too.

travier avatar Dec 13 '22 17:12 travier

So we're moving from one place where it is set to multiple places needing it passed.

travier avatar Dec 13 '22 17:12 travier

I would agree to move all tools to reading the value from the build meta.json as much as possible but I still think it's simpler for the developer to set it once at cosa init time.

travier avatar Dec 13 '22 17:12 travier

To elaborate, the main concern I have with src/config.json is that it creates two sources of truth on what variant we're working with. Apart from a few key commands, most commands operate on an existing build where I think we need to respect its variant.

If we want to keep src/config.json and declare that "only cosa fetch and cosa build will read it", that's OK. But it requires maintaining that distinction. My thought with removing it is that then we never have to worry about code using it when it should use build metadata instead (like cosa buildextend-extensions-container and kola).

Hmm, maybe we can keep it in cosa init but name the file e.g. src/.building-variant and then cosa build deletes it on success. WDYT?

And reading the bump-lockfile job, it looks like we'll need that for the buildfetch command too.

Not sure I follow; why does cosa buildfetch need it?

jlebon avatar Dec 13 '22 21:12 jlebon

To elaborate, the main concern I have with src/config.json is that it creates two sources of truth on what variant we're working with. Apart from a few key commands, most commands operate on an existing build where I think we need to respect its variant.

Definitely agree.

If we want to keep src/config.json and declare that "only cosa fetch and cosa build will read it", that's OK. But it requires maintaining that distinction. My thought with removing it is that then we never have to worry about code using it when it should use build metadata instead (like cosa buildextend-extensions-container and kola).

Hmm, maybe we can keep it in cosa init but name the file e.g. src/.building-variant and then cosa build deletes it on success. WDYT?

+1 for moving to a hidden file.

I would prefer to keep it even on successful builds so that all cosa fetch/build commands in a given cloned dir always give out the same variant. We could look at the previous build to figure out the variant to build but that would not be preserved if you do a cosa clean. If would then be non-obvious what to do to get the variant back apart from doing a cosa init again.

So another option would be to add another command that just sets the variant that will be build in this file so that it's not decided on cosa init and you can change the variant for the next builds / after a clean with it.

And reading the bump-lockfile job, it looks like we'll need that for the buildfetch command too.

Not sure I follow; why does cosa buildfetch need it?

OK, buildfetch gets passed the "stream" dir directly so it would not need to be variant aware: https://github.com/coreos/fedora-coreos-pipeline/blob/a37d3f7c93339c6ab308bcf385bba40f866be6d3/jobs/kola-gcp.Jenkinsfile#L62

travier avatar Dec 14 '22 12:12 travier

$ cosa init ...
# Set it once
$ cosa set-variant rhel-9.0
$ cosa fetch
$ cosa build
$ cosa clean
$ cosa build
# Change what to build
$ cosa set-variant c9s
...

travier avatar Dec 14 '22 12:12 travier

We could look at the previous build to figure out the variant to build but that would not be preserved on if you do a cosa clean.

Right, though cosa clean could actually take the variant data out of the last build, and put it back in the init directory.

cgwalters avatar Dec 14 '22 13:12 cgwalters

That could work but we would still need the marker file at the beginning so why not keep it all the time, used only by fetch & build operations?

travier avatar Dec 16 '22 10:12 travier

Cool with keeping it, but maybe we can name it something like .cosa-build-variant? We could also add a field to the JSON schema like "note": "this file should only be used by cosa fetch and cosa build". We do something similar for the release index in FCOS.

jlebon avatar Dec 16 '22 18:12 jlebon