-
Notifications
You must be signed in to change notification settings - Fork 14
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
Generate stream metadata from releases #1
Conversation
main.go
Outdated
os.Exit(1) | ||
} | ||
|
||
defer file.Close() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
release.go
Outdated
Release string `json:"release"` | ||
Stream string `json:"stream"` | ||
Metadata Metadata `json:"metadata"` | ||
Architectures ReleaseArchitectures `json:"architectures"` |
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.
Might be worthwhile to use a map of strings to a generic architecture type over a wrapper struct.
|
||
defer file.Close() | ||
|
||
if overrideFilename != "" { |
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.
Is it intended that stream.json
is still produced if overrideFilename
is specified? I would expect that if specified the output would be routed to that location instead of stream.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.
As far as I understand yes, stream.json will be automatically generated in both cases - a) when release.json is updated b) when an override file is specified.
As per coreos/fedora-coreos-tracker#192 (comment) , stream.json is what user will be referring to download latest media . Override file be a partial file(following stream,json schema) which will be used only when we want users to download some other version than what is available in release.json .Some detail on override file is in coreos/fedora-coreos-tracker#98 . @bgilbert did I get things right?
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.
Ah, I misread this initially as an additional output file. Don't mind me.
Thanks @lucab and @arithx for review! Addressed review comment in sinnykumari@2899dae |
main.go
Outdated
|
||
if releaseArch.Media.Azure.Images.Global != nil { | ||
if releaseArch.Media.Azure.Images != nil && |
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.
The golang style of formatting makes this block a bit hard to read at a glance. Might read better doing something like so:
if az := releaseArch.Media.Azure.Images; az != nil && az.Global != nil && az.Global.Image != nil {
...
}
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.
Good idea, fixed it!
return "", err | ||
} | ||
|
||
return relIndex.Releases[len(relIndex.Releases)-1].Metadata, nil |
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.
It would be better to have a check that len(relIndex.Releases) > 0
, this may panic.
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.
Indeed, nice catch. fixed!
main.go
Outdated
|
||
func main() { | ||
err := run() | ||
if err != nil { |
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.
Minor style suggestion: you can write this as if err := run(); err != nil {...}
.
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 am still picking up Go, thanks for the suggestion, fixed!
Just two minor things, looks good otherwise. I didn't spot any logic mistake in here, but I didn't look too hard... |
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.
Just some minor comments from a quick scan!
I tested this against the meta translation tool I'm writing and it worked. I would suggest a patch dropping the Here's an example of the patch I hacked together:
|
Thanks @arithx for the patch! it looks good and have pushed same in sinnykumari@5e56fa0 |
Can we have a final review on this PR before we merge it to master? |
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.
Just one minor comment, otherwise LGTM!
} | ||
|
||
// If outputFile option not specified print on stdout | ||
if outputFile != "" { |
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.
Minor: could dedupe this logic by having e.g. an out
variable that you set to either os.Stdout
or streamFile
.
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.
Thanks, fixed it
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.
Last comment: in the interest of keeping the git history clean, WDYT about squashing it all to a single "initial commit" patch to seed master with? I find my initial git commits are usually of low quality as I hack things together, and so becomes mostly noise.
Up to you though, it mostly comes down to personal preference. :)
I am also in favor if keeping git history clean. Squashed all commits to single commit. Will wait for some time before merging to address any final review comment |
merged, thanks all for review! |
No description provided.