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.

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

Move variant setup from cosa init to cosa build #3276

Open
travier opened this issue Dec 13, 2022 · 10 comments
Open

Move variant setup from cosa init to cosa build #3276

travier opened this issue Dec 13, 2022 · 10 comments
Labels
design design discussion enhancement New feature or request jira for syncing to jira

Comments

@travier
Copy link
Member

travier commented Dec 13, 2022

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:

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
Copy link
Member Author

travier commented Dec 13, 2022

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 travier added jira for syncing to jira enhancement New feature or request design design discussion labels Dec 13, 2022
@travier
Copy link
Member Author

travier commented Dec 13, 2022

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

@travier
Copy link
Member Author

travier commented Dec 13, 2022

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

@travier
Copy link
Member Author

travier commented Dec 13, 2022

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.

@jlebon
Copy link
Member

jlebon commented Dec 13, 2022

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?

@travier
Copy link
Member Author

travier commented Dec 14, 2022

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
Copy link
Member Author

travier commented Dec 14, 2022

$ 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
...

@cgwalters
Copy link
Member

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.

@travier
Copy link
Member Author

travier commented Dec 16, 2022

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?

@jlebon
Copy link
Member

jlebon commented Dec 16, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design design discussion enhancement New feature or request jira for syncing to jira
Projects
None yet
Development

No branches or pull requests

3 participants