-
Notifications
You must be signed in to change notification settings - Fork 76
cosa: use generated structs from schema #1187
Conversation
Dependant PR coreos/coreos-assembler#1147 Throwing this out for early review before I get too far into this. No idea if this works....YMMV if you try this. |
Oh neat, I didn't know about schematyper. |
Okay, the latest push works. @arithx WDDYT about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One open question otherwise LGTM.
schema_commit="08725e679fbc3b923a8f7196a9ac505263db7c07" | ||
schema_url="https://raw.githubusercontent.com/coreos/coreos-assembler/${schema_commit}/src/schema/${schema_version}.json" | ||
echo "Generating COSA Schema ${schema_version} ${schema_commit}" | ||
curl -sL "${schema_url}" -o "cosa/${schema_version}.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wonder if there's value in just leaving the act of fetching the schema from the COSA github to a manual process rather than fetching it automatically each run of generate
, especially given we're including the file in this repository (cosa/v1.json
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, the question I've been asking -- and I have an answer that I like, per se -- do we:
- leave the schema, and generated GoLang in COSA itself
- vendor it in, ala
go get github.com/coreos/coreos-assembler/schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either would work for me. At that point it's more of a matter of if we want to carry golang code in COSA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes this more confusing though is that mantle itself is a submodule of cosa. So we can end up in a situation where the baked mantle is using a different schema than the rest of cosa. (Of course, all this just hints at what we talking about re. either merging the two together, or alternatively separating them better.)
Hmm, maybe what we actually want here is to do all this at the ./build
step? E.g. ./build
in this repo defaults to using the schema from git master cosa, but when built as a submodule in cosa, we can pass the file to the schema to translate so that we're guaranteed they're always in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, coreos/coreos-assembler#163 and I also added this as an item for realtime cabal discussion tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the cabal topic is a marvelous idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to block this on that though IMO, it's just one of a number of things that would be changed/tweaked after a merge, and if we don't merge it's still worth it to land this.
CoreOS Assembler recently got a new schema. Rather than use hand-generated structs, this change uses schematyper to generate the code.
Use the canonical schema from COSA.