Move the schema/ package to the toplevel #3069

cgwalters · 2022-09-02T17:45:07Z

Today, we have three Go modules in this repository:

/
schema/
mantle/

And as of recently, both the toplevel / and mantle/ vendor the schema/.
This causes "vendor amplification", where a vendored dependency
of schema/ gets triplicated.

Now that gangplank/ is out of the picture, it makes sense to move
the bits from schema/ into the toplevel module.

This really helps make operating on coreos-assembler stuff
feel like a "first class" operation.

Also, we no longer have two copies of the schema file!

openshift-ci · 2022-09-02T17:45:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cgwalters · 2022-09-06T13:22:17Z

One way to look at this is if it'd merged before #2985 the number of files changed there would have been closer to 4, not 400+. The 400+ files removed here are the (re-)vendored dependencies there.

cgwalters · 2022-09-06T14:44:10Z

OK cool, passing FCOS/Jenkins CI now, I'd assume RHCOS will pass too. Should be good to review

cgwalters · 2022-09-07T16:52:45Z

/retest

mantle/vendor/github.com/coreos/coreos-assembler/LICENSE

dustymabe · 2022-09-07T18:03:56Z

A few things.

Are there any consumers of this package outside of this repo?
I wonder if we should consider naming here more critically.
- I don't know if having a ./cosa top level directory in this repo is best. Maybe ./schema? i.e. github.com/coreos/coreos-assembler/pkg/cosa has both coreos-assembler and cosa so it's hard to infer what it even is. github.com/coreos/coreos-assembler/pkg/schema may be a bit more intuitive?

cgwalters · 2022-09-07T18:31:52Z

The "schema" package before also had code to parse builds.json etc. (duplicating what we have in the python side of course) so it isn't just schema.

cgwalters · 2022-09-07T18:41:08Z

Are there any consumers of this package outside of this repo?

Not to my knowledge, but certainly as we make the transition more into being a Go project, we should think about this.

It's worth looking at https://github.com/golang-standards/project-layout

However actually, because mantle is a separate Go module, even though it lives in the same git repository we need to make packages it uses public - for now.

cgwalters · 2022-09-07T18:41:53Z

I don't know if having a ./cosa top level directory in this repo is best. Maybe ./schema? i.e. github.com/coreos/coreos-assembler/pkg/cosa has both coreos-assembler and cosa so it's hard to infer what it even is.

Perhaps pkg/builds is best? That seems to match the naming we use on the python side.

dustymabe · 2022-09-07T19:03:40Z

I don't know if having a ./cosa top level directory in this repo is best. Maybe ./schema? i.e. github.com/coreos/coreos-assembler/pkg/cosa has both coreos-assembler and cosa so it's hard to infer what it even is.

Perhaps pkg/builds is best? That seems to match the naming we use on the python side.

It's as good as any I can think of. Maybe we can get input from others?

Side note: I'm not going to lie, the word schema invokes a clear idea of what it is so I liked that. I didn't realize there was much else bundled in there with it. I'm guessing it's outside the scope here to peel off the builds part into pkg/builds and leave the schema part in a pkg/schema?

cgwalters · 2022-09-07T21:27:24Z

I'm guessing it's outside the scope here to peel off the builds part into pkg/builds and leave the schema part in a pkg/schema?

Right...there's always more cleanup to do. As a baseline I tend to ask "is this PR making things objectively better?" And if so then I prefer merging and doing followup changes, especially from people I know are likely to stick around to do those changes. Actually even for drive by contributors, merging slightly-less-than-perfect changes helps avoid the case where they're pulled away by life events or whatever and the PR doesn't get merged at all (or not for a while).

cgwalters · 2022-09-07T22:10:06Z

OK renamed to pkg/builds

dustymabe · 2022-09-07T23:25:06Z

mantle/cmd/kola/kola.go

@@ -31,7 +31,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"

-	"github.com/coreos/coreos-assembler-schema/cosa"
+	cosa "github.com/coreos/coreos-assembler/pkg/builds"


random aside: I'm wondering if we should not rename the import here.. not sure. I'm definitely don't have this all in my head.

It's a matter of taste; I chose not to here to avoid inflating the diff more. Probably in most code we wouldn't rename it though.

dustymabe · 2022-09-07T23:26:47Z

there is a build.goe file in the diff now. was that on purpose?

dustymabe · 2022-09-07T23:27:59Z

there is a build.goe file in the diff now. was that on purpose?

nvm - there's a bunch of them. Something new for me to learn I guess.

Any go experts want to review this PR along with me?

cgwalters · 2022-09-08T00:52:51Z

there is a build.goe file in the diff now. was that on purpose?

Oops definitely not, it was apparently a misfired sed command to try to s/package cosa/package builds/...

cgwalters · 2022-09-09T11:51:06Z

While it's not related to this PR in a logical sense, I pushed an additional commit here which should fix cosa build-extensions-container on aarch64. The commit would textually conflict with the move otherwise.

dustymabe · 2022-09-09T17:29:21Z

Is the src/v1.json file needed? We didn't have that before, right?

cgwalters · 2022-09-09T17:31:45Z

Is the src/v1.json file needed? We didn't have that before, right?

We did; see https://github.com/coreos/coreos-assembler/blob/main/src/v1.json
in current git main.

What changes in this PR is we no longer have two separate copies.

dustymabe · 2022-09-09T17:38:35Z

Is the src/v1.json file needed? We didn't have that before, right?

We did; see https://github.com/coreos/coreos-assembler/blob/main/src/v1.json in current git main.

Ahh. Yep. The diff view here in GH didn't make that obvious.

What changes in this PR is we no longer have two separate copies.

It's great that we now don't have two separate copies, but I am worried this is going to break a common COSA workflow where someone sets the COREOS_ASSEMBLER_GIT env var.

cgwalters · 2022-09-09T17:49:02Z

I am worried this is going to break ... the COREOS_ASSEMBLER_GIT env var.

With the move to Go, that workflow will slowly become obsolete and break. Eventually I think what we want is that literally all of the logic lives in /usr/bin/coreos-assembler as a big static binary that you can just...copy around or bind mount in or whatever.

But specifically regarding the schema, even before this PR that problem already existed because you couldn't modify it and still invoke e.g. cosa run. It'd only work as long as the only things reading/writing were python and shell.

dustymabe · 2022-09-09T17:51:53Z

It'd only work as long as the only things reading/writing were python and shell.

which is the use case that COREOS_ASSEMBLER_GIT satisfies.

I'd say until we get a better strategy for how we support that use case in the future maybe let's just make the schema generation Makefile put a copy in both places.. Alternatively put the actual file in src/v1.json and symlink the other one.

Today, we have *three* Go modules in this repository: - / - schema/ - mantle/ And as of recently, both the toplevel / and mantle/ vendor the schema/. This causes "vendor amplification", where a vendored dependency of schema/ gets *triplicated*. Now that gangplank/ is out of the picture, it makes sense to move the bits from schema/ into the toplevel module. This really helps make operating on coreos-assembler stuff feel like a "first class" operation. Also, we no longer have two copies of the schema file!

A perfect example of the enormous technical debt we've accrued in the python/shell/Go duplication, not to mention separate Go modules for mantle and gangplank. The gangplank code reimplemented logic to get the RPM architecture, and missed converting `arm64` from Go's arch to `aarch64`. Use the standard API we have for this in stream-metadata-go, the same as mantle does. This will fix extensions builds on aarch64.

cgwalters · 2022-09-09T17:56:59Z

Alternatively put the actual file in src/v1.json and symlink the other one.

Sure, done - that actually makes the install logic simpler.

dustymabe

LGTM

cgwalters · 2022-09-09T18:10:48Z

I'd say until we get a better strategy for how we support that use case in the future

The future is now ➡️ #3087

Fixes: coreos#3069 I am pretty sure I had this working in some point in that PR, but in reworking for feedback I broke having `make schema` actually work. Put the target files in the right place, and fix the namespace.

Fixes: #3069 I am pretty sure I had this working in some point in that PR, but in reworking for feedback I broke having `make schema` actually work. Put the target files in the right place, and fix the namespace.

openshift-ci bot added the do-not-merge/work-in-progress label Sep 2, 2022

cgwalters force-pushed the schema-top branch 3 times, most recently from b2c59d0 to 169772c Compare September 6, 2022 12:04

cgwalters marked this pull request as ready for review September 6, 2022 12:04

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 6, 2022

cgwalters force-pushed the schema-top branch 2 times, most recently from 70af286 to dbd5ad7 Compare September 6, 2022 13:08

openshift-merge-robot added the needs-rebase label Sep 7, 2022

cgwalters force-pushed the schema-top branch from dbd5ad7 to 6f26716 Compare September 7, 2022 12:14

openshift-merge-robot removed the needs-rebase label Sep 7, 2022

dustymabe reviewed Sep 7, 2022

View reviewed changes

mantle/vendor/github.com/coreos/coreos-assembler/LICENSE Show resolved Hide resolved

cgwalters force-pushed the schema-top branch from 6f26716 to 29af1ae Compare September 7, 2022 22:10

dustymabe reviewed Sep 7, 2022

View reviewed changes

cgwalters force-pushed the schema-top branch from 29af1ae to 6a89503 Compare September 8, 2022 00:52

cgwalters force-pushed the schema-top branch from d76f3eb to 8d35527 Compare September 9, 2022 11:35

cgwalters added 2 commits September 9, 2022 13:55

cgwalters force-pushed the schema-top branch from 8d35527 to c5f25ec Compare September 9, 2022 17:55

dustymabe approved these changes Sep 9, 2022

View reviewed changes

cgwalters enabled auto-merge (rebase) September 9, 2022 18:05

cgwalters merged commit 493c480 into coreos:main Sep 9, 2022

cgwalters mentioned this pull request Sep 12, 2022

schema: Fix make schema #3092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the schema/ package to the toplevel #3069

Move the schema/ package to the toplevel #3069

cgwalters commented Sep 2, 2022 •

edited

Loading

openshift-ci bot commented Sep 2, 2022

cgwalters commented Sep 6, 2022 •

edited

Loading

cgwalters commented Sep 6, 2022

cgwalters commented Sep 7, 2022

dustymabe commented Sep 7, 2022

cgwalters commented Sep 7, 2022

cgwalters commented Sep 7, 2022

cgwalters commented Sep 7, 2022

dustymabe commented Sep 7, 2022

cgwalters commented Sep 7, 2022

cgwalters commented Sep 7, 2022

dustymabe Sep 7, 2022

cgwalters Sep 8, 2022

dustymabe commented Sep 7, 2022

dustymabe commented Sep 7, 2022

cgwalters commented Sep 8, 2022

cgwalters commented Sep 9, 2022

dustymabe commented Sep 9, 2022

cgwalters commented Sep 9, 2022

dustymabe commented Sep 9, 2022

cgwalters commented Sep 9, 2022

dustymabe commented Sep 9, 2022

cgwalters commented Sep 9, 2022

dustymabe left a comment

cgwalters commented Sep 9, 2022

Move the schema/ package to the toplevel #3069

Move the schema/ package to the toplevel #3069

Conversation

cgwalters commented Sep 2, 2022 • edited Loading

openshift-ci bot commented Sep 2, 2022

cgwalters commented Sep 6, 2022 • edited Loading

cgwalters commented Sep 6, 2022

cgwalters commented Sep 7, 2022

dustymabe commented Sep 7, 2022

cgwalters commented Sep 7, 2022

cgwalters commented Sep 7, 2022

cgwalters commented Sep 7, 2022

dustymabe commented Sep 7, 2022

cgwalters commented Sep 7, 2022

cgwalters commented Sep 7, 2022

dustymabe Sep 7, 2022

Choose a reason for hiding this comment

cgwalters Sep 8, 2022

Choose a reason for hiding this comment

dustymabe commented Sep 7, 2022

dustymabe commented Sep 7, 2022

cgwalters commented Sep 8, 2022

cgwalters commented Sep 9, 2022

dustymabe commented Sep 9, 2022

cgwalters commented Sep 9, 2022

dustymabe commented Sep 9, 2022

cgwalters commented Sep 9, 2022

dustymabe commented Sep 9, 2022

cgwalters commented Sep 9, 2022

dustymabe left a comment

Choose a reason for hiding this comment

cgwalters commented Sep 9, 2022

cgwalters commented Sep 2, 2022 •

edited

Loading

cgwalters commented Sep 6, 2022 •

edited

Loading