cmd/plume/fcos: modify build metadata structure #1015
Conversation
cmd/plume/types.go
Outdated
| Endpoint string `json:"endpoint"` | ||
| CommitHash []Commit `json:"commits"` | ||
| Version string `json:"version"` | ||
| Endpoint string `json:"endpoint"` |
There was a problem hiding this comment.
I think this is called metadata in the JSON field over here coreos/fedora-coreos-tracker#98 (comment).
| } | ||
|
|
||
| type Metadata struct { | ||
| LastModified string `json:"last-modified"` | ||
| } | ||
|
|
||
| type IndividualReleaseMetadata struct { |
There was a problem hiding this comment.
I think at some later point (even in another PR) it would make sense to split structures belonging to different JSON-documents in different source files. It took me a bit to realize where this one was coming from.
There was a problem hiding this comment.
I feel like that might cause confusion though if the FCOS metadata types are defined in separate files while every other struct used in plume is inside of this file though. Is this maybe more of an issue with the individual names of these structs rather than the location?
There was a problem hiding this comment.
I'm fine either ways (and also keeping as-is). It was just a minor sentiment when scanning through the source.
cmd/plume/release.go
Outdated
| if len(a) > len(b) { | ||
| return 1 | ||
| } | ||
| equal := len(a) == len(b) |
There was a problem hiding this comment.
s/equal/sameLenght/ (or something similar, just for clarity)
There was a problem hiding this comment.
wondering should we keep "note" or remove it from ReleaseMetadata https://github.com/coreos/mantle/blob/master/cmd/plume/types.go#L89 ?
It was decided upstream to have the |
Ok, should we get it added into the release-index coreos/fedora-coreos-tracker#98 (comment) ? |
I don't think it's strictly necessary as this is mostly just present in the case that something other than tools stumbles upon the file to dissuade usage. Any tool (e.x. the stream metadata generator) doesn't need to know or care about this field when parsing the release metadata index. |
sure, mentioned it to make sure we didn't miss anything :) |
Per discussions in the [fedora-coreos-tracker](coreos/fedora-coreos-tracker#98 (comment)), modify the build metadata structure to better support multi-arch.
|
Updated addressing comments. |
|
LGTM. If you are not in a hurry I'd suggest leaving this hanging till tomorrow so @sinnykumari has a last chance for a pass. |
|
@lucab yup, was planning on waiting for everyone involved to have the chance to take a peek. |
Per discussions in the fedora-coreos-tracker, modify the build metadata structure to better support multi-arch.