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

cosa/cli-spec: add envVar CLI support #1

Merged
merged 8 commits into from
Aug 21, 2020
Merged

cosa/cli-spec: add envVar CLI support #1

merged 8 commits into from
Aug 21, 2020

Conversation

darkmuggle
Copy link
Contributor

Introduction of a clispec for converting YAML to Cli arguments.

cosa/cli-spec.md Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

cosa/cli-spec.md

Can we prefix these with the date they were proposed? i.e. cosa/20200804-cli-spec.md? To me it really helps over time to have some sort of order among things like this, even if it is arbitrary. The date is as good of ordering scheme as any I can think of to give these files some context.

@cgwalters
Copy link
Member

Seems OK to me as is. Optionals:

"cli spec" seems somewhat odd as a name, maybe "buildconfig"?

But I think an interesting thread here is around if we end up splitting up the pipeline into phases; for example, only doing cloud image uploads after other basic sanity checking (or making images public later); would it make more sense to have the "spec/config" be higher level, e.g.

extend:
  - aws
  - gcp

is a distinct thing from build which would imply creating a new ostree commit or so?

Which would differ from build in that it would require a previously fetched build that gets extended.

@lucab
Copy link

lucab commented Aug 5, 2020

I like the manifest approach, but I don't agree on the YAML<->env mapping. In particular, those are not isomorphic structures so it gets really messy when you got into handling more complex cases that the examples here are glancing over (list-encoding, empty value vs missing value, tables nested more than one level, etc). I would strongly recommend not go that route, because it's a lot of self-inflicted pain due to an improper abstraction translation (here the same topic with more details, in a different context).

What price would we have to pay to avoid that bridging logic? My feeling is that is mostly intended as a mechanism to slide from the old to the new logic without breaking consumers, is that right?

@darkmuggle
Copy link
Contributor Author

darkmuggle commented Aug 5, 2020

What price would we have to pay to avoid that bridging logic? My feeling is that is mostly intended as a mechanism to slide from the old to the new logic without breaking consumers, is that right?

Yup.

The short story is that there is almost no cohesion in COSA between the components. The closest we came was during the now-abandoned rework of COSA. Since each command essentially does its own thing, this was the lowest common denominator. Short of re-starting the COSA rework with a cogent interface, this was the sanest design I could come up with: a hacky bolt on.

FWIW, with an eye towards iterative improvement, I proposed using one thing to handle the translation instead of every component doing itself.

@darkmuggle
Copy link
Contributor Author

Updated with type examples to address some of what @lucab highlighted.

Signed-off-by: Ben Howard <ben.howard@redhat.com>
@darkmuggle
Copy link
Contributor Author

The alternative that I considered was having an entrypoint into COSA that rendered the manifest into CLI arguments that then were executed. I figured that this idea is less bad.

Regardless I am seriously wondering whether the effort is worth it. The RHCOS job spec won't translate well into this format and FCOS would have to learn it (assuming there was any appetite for this interface in the first place).

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.

I was looking over the FCOS pipeline code to try to imagine what it would look like with this proposal. One thing that jumped out that I think is worth highlighting is that as opposed to RHCOS, the FCOS pipeline today doesn't have per-stream pipeline configs. So this ends up meaning that a lot of the values are calculated dynamically (e.g. the ref to check out, the S3 stream directory, GCP family/license, RoboSignatory flags).

What I'd like to avoid is having pipeline code write to the config file which then gets passed to cosa. So I think with this approach, it's pushing FCOS towards matching what RHCOS is doing (and of course that's part of the goal, but it's worth expanding), which is having per-stream config files. So e.g. they'd live in their individual branches in https://github.com/coreos/fedora-coreos-config (or maybe it'd be cleaner to just have them sit alongside each other in the master branch of https://github.com/coreos/fedora-coreos-pipeline). And we'd then drop the dynamic logic in the pipeline.

And of course, there are pros and cons to each approach. Having it be dynamic the way it currently is means that it's less config file maintenance and lower chances of unwanted skew between streams, while having it written down in config files means it's easier to change things for a single stream and ideally reduce pipeline complexity. Maybe eventually we can have some kind of "config inheritance" to help dedupe the identical stuff across streams and have each stream-specific config focus on differences only? (This is similar to how manifest.yaml works in FCOS today, see https://github.com/coreos/fedora-coreos-config#about-this-repo).

Anyway, I'm overall +1 on this proposal, and while I don't think the FCOS pipeline suffers from a lack of this yet, having it align better with the RHCOS pipeline is valuable and a prerequisite towards further alignment.

cosa/20200804-cfg-spec.md Outdated Show resolved Hide resolved
cosa/20200804-cfg-spec.md Outdated Show resolved Hide resolved
cosa/20200804-cfg-spec.md Outdated Show resolved Hide resolved
cosa/20200804-cfg-spec.md Outdated Show resolved Hide resolved
cosa/20200804-cfg-spec.md Outdated Show resolved Hide resolved

The innovation of the RHCOS JobSpec was it provided a clear seperation of configuration from the code; the logic was conflated. Further the jobspec allowed the logic to be run with different inputs.

To support this, a "COSAspec" is being introduced to provide a CLI file-interface.
Copy link
Member

Choose a reason for hiding this comment

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

Can you include a config name for the YAML file so that we can bikeshed on it? 🎨
I'll throw in a suggestion for cosa-config.yaml.

Also, this proposal doesn't cover where this config needs to live. Is it auto-discovered by cosa? If so, where does it look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally left that the where and name of the config out. The rationale is that the COSAspec YAML is something that is brought to COSA. If anything I would run with the idea of it coming from an external repo (e.g. f.c.c or rh.c.c).

Therefore, for each command updated to modify this interface:
- The prefix `COSA_<COSA COMMAND>` will be used for envVars (e.g. `COSA_EXTEND_AZURE`)
- No calculated values. Commands may not parse the spec file itself.
- One command per PR.
Copy link
Member

Choose a reason for hiding this comment

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

I think to start with, we should keep the scope mostly to the CLI switches we actually really care about for pipelines. Some things are inherently dynamic, e.g. FCOS's cosa build --parent config, and some things are really just developer/instance specific (e.g. cosa build --tag).

Let's add that to the proposal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea -- and that's related to why I stated a "PR standard." Dropping a huge PR would be ugly and IMO there are a bunch of commands we don't want this interface for.

Ben Howard and others added 6 commits August 5, 2020 15:11
Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

👍

@darkmuggle
Copy link
Contributor Author

I don't think this spec is needed, but I don't see any harm in having this in case we do need it later.

@darkmuggle darkmuggle merged commit 49f70aa into coreos:master Aug 21, 2020
@darkmuggle darkmuggle deleted the pr/cosa/cli-spec branch August 21, 2020 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants