Generate stream metadata from releases #1

sinnykumari · 2019-06-28T13:38:10Z

No description provided.

main.go

+		os.Exit(1)
+	}
+
+	defer file.Close()


arithx · 2019-06-28T17:00:09Z

release.go

+	Release       string               `json:"release"`
+	Stream        string               `json:"stream"`
+	Metadata      Metadata             `json:"metadata"`
+	Architectures ReleaseArchitectures `json:"architectures"`


Might be worthwhile to use a map of strings to a generic architecture type over a wrapper struct.

arithx · 2019-06-28T17:22:27Z

main.go

+
+	defer file.Close()
+
+	if overrideFilename != "" {


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.

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?

Ah, I misread this initially as an additional output file. Don't mind me.

sinnykumari · 2019-07-01T10:06:28Z

Thanks @lucab and @arithx for review! Addressed review comment in sinnykumari@2899dae

arithx · 2019-07-01T16:22:08Z

main.go


-		if releaseArch.Media.Azure.Images.Global != nil {
+		if releaseArch.Media.Azure.Images != nil &&


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 { ... }

Good idea, fixed it!

lucab · 2019-07-01T16:57:05Z

release_index.go

+		return "", err
+	}
+
+	return relIndex.Releases[len(relIndex.Releases)-1].Metadata, nil


It would be better to have a check that len(relIndex.Releases) > 0, this may panic.

Indeed, nice catch. fixed!

lucab · 2019-07-01T16:58:23Z

main.go

+
+func main() {
+	err := run()
+	if err != nil {


Minor style suggestion: you can write this as if err := run(); err != nil {...}.

I am still picking up Go, thanks for the suggestion, fixed!

lucab · 2019-07-01T17:00:49Z

Just two minor things, looks good otherwise. I didn't spot any logic mistake in here, but I didn't look too hard...

jlebon

Just some minor comments from a quick scan!

main.go

arithx · 2019-07-05T15:31:49Z

I tested this against the meta translation tool I'm writing and it worked. I would suggest a patch dropping the Artifacts type and using a map[string]*ImageFormat where you're currently declaring them. That way the Artifacts type doesn't have to specify every single format for them to be picked up.

Here's an example of the patch I hacked together:

From 84c61de26b553b775bff1eebb358e95129bbf52c Mon Sep 17 00:00:00 2001
From: Stephen Lowrie
Date: Fri, 5 Jul 2019 10:27:15 -0500
Subject: [PATCH] *: switch artifacts to map

This allows the tool to pick up different artifact formats without
needing to add additional keys.
---
 common.go  | 14 --------------
 release.go | 16 ++++++++--------
 stream.go  |  4 ++--
 3 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/common.go b/common.go
index 310266d..6af7481 100644
--- a/common.go
+++ b/common.go
@@ -5,20 +5,6 @@ type Metadata struct {
 	LastModified string `json:"last-modified"`
 }
 
-// Artifacts contains various artifacts
-type Artifacts struct {
-	Raw     *ImageFormat `json:"raw.xz,omitempty"`
-	Qemu    *ImageFormat `json:"qcow.xz,omitempty"`
-	Ova     *ImageFormat `json:"ova,omitempty"`
-	Tar     *ImageFormat `json:"tar.gz,omitempty"`
-	Vmdk    *ImageFormat `json:"vmdk.xz,omitempty"`
-	Vdi     *ImageFormat `json:"vdi.xz,omitempty"`
-	Iso     *ImageFormat `json:"iso,omitempty"`
-	Pxe     *ImageFormat `json:"pxe,omitempty"`
-	InstIso *ImageFormat `json:"installer.iso,omitempty"`
-	InstPxe *ImageFormat `json:"installer-pxe,omitempty"`
-}
-
 // ImageFormat contains Disk image details
 type ImageFormat struct {
 	Disk      *ImageType `json:"disk,omitempty"`
diff --git a/release.go b/release.go
index 6e14ac8..56d3ffe 100644
--- a/release.go
+++ b/release.go
@@ -29,20 +29,20 @@ type ReleaseMedia struct {
 
 // ReleaseAws contains AWS image information
 type ReleaseAws struct {
-	Artifacts Artifacts                      `json:"artifacts"`
+	Artifacts map[string]*ImageFormat        `json:"artifacts"`
 	Images    *map[string]*ReleaseCloudImage `json:"images"`
 }
 
 // ReleaseDigitalOcean DigitalOcean image detail
 type ReleaseDigitalOcean struct {
-	Artifacts Artifacts `json:"artifacts"`
-	Image     string    `json:"image"`
+	Artifacts map[string]*ImageFormat `json:"artifacts"`
+	Image     string                  `json:"image"`
 }
 
 // ReleaseAzure Azure image detail
 type ReleaseAzure struct {
-	Artifacts Artifacts           `json:"artifacts"`
-	Images    *ReleaseAzureImages `json:"images"`
+	Artifacts map[string]*ImageFormat `json:"artifacts"`
+	Images    *ReleaseAzureImages     `json:"images"`
 }
 
 // ReleaseAzureImages Azure image detail
@@ -52,8 +52,8 @@ type ReleaseAzureImages struct {
 
 // ReleaseGcp GCP image detail
 type ReleaseGcp struct {
-	Artifacts Artifacts `json:"artifacts"`
-	Image     *string   `json:"image"`
+	Artifacts map[string]*ImageFormat `json:"artifacts"`
+	Image     *string                 `json:"image"`
 }
 
 // ReleaseCloudImage cloud image information
@@ -63,5 +63,5 @@ type ReleaseCloudImage struct {
 
 // ReleaseTargetPlatform target platforms
 type ReleaseTargetPlatform struct {
-	Artifacts Artifacts `json:"artifacts"`
+	Artifacts map[string]*ImageFormat `json:"artifacts"`
 }
diff --git a/stream.go b/stream.go
index 23fded1..4f6a3cc 100644
--- a/stream.go
+++ b/stream.go
@@ -30,8 +30,8 @@ type StreamArtifacts struct {
 
 // StreamMediaDetails contains image artifact and release detail
 type StreamMediaDetails struct {
-	Release string    `json:"release"`
-	Formats Artifacts `json:"formats"`
+	Release string                  `json:"release"`
+	Formats map[string]*ImageFormat `json:"formats"`
 }
 
 // StreamImages contains images available in cloud providers
-- 
2.21.0

sinnykumari · 2019-07-10T10:15:20Z

Thanks @arithx for the patch! it looks good and have pushed same in sinnykumari@5e56fa0

sinnykumari · 2019-07-10T10:32:46Z

Can we have a final review on this PR before we merge it to master?

jlebon

Just one minor comment, otherwise LGTM!

jlebon · 2019-07-11T13:36:06Z

main.go

+	}
+
+	// If outputFile option not specified print on stdout
+	if outputFile != "" {


Minor: could dedupe this logic by having e.g. an out variable that you set to either os.Stdout or streamFile.

Thanks, fixed it

jlebon

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. :)

sinnykumari · 2019-07-11T15:07:53Z

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

sinnykumari · 2019-07-11T15:39:03Z

merged, thanks all for review!

lucab reviewed Jun 28, 2019

View reviewed changes

main.go Outdated

os.Exit(1)

}

defer file.Close()

This comment was marked as resolved.

Sign in to view

arithx reviewed Jun 28, 2019

View reviewed changes

arithx reviewed Jul 1, 2019

View reviewed changes

lucab reviewed Jul 1, 2019

View reviewed changes

sinnykumari force-pushed the init branch from 2899dae to e373814 Compare July 1, 2019 17:33

jlebon reviewed Jul 2, 2019

View reviewed changes

main.go Outdated Show resolved Hide resolved

main.go Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

sinnykumari force-pushed the init branch from 39f3f04 to db7cfe5 Compare July 10, 2019 10:32

jlebon reviewed Jul 11, 2019

View reviewed changes

jlebon approved these changes Jul 11, 2019

View reviewed changes

Initial implementation of stream metadata generator

714e1b7

sinnykumari force-pushed the init branch from a9b28c8 to 714e1b7 Compare July 11, 2019 15:05

sinnykumari merged commit 725ea8b into master Jul 11, 2019

dustymabe deleted the init branch May 11, 2021 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate stream metadata from releases #1

Generate stream metadata from releases #1

sinnykumari commented Jun 28, 2019

This comment was marked as resolved.

arithx Jun 28, 2019

arithx Jun 28, 2019

sinnykumari Jul 1, 2019 •

edited

Loading

arithx Jul 1, 2019

sinnykumari commented Jul 1, 2019

arithx Jul 1, 2019

sinnykumari Jul 1, 2019

lucab Jul 1, 2019

sinnykumari Jul 1, 2019

lucab Jul 1, 2019

sinnykumari Jul 1, 2019

lucab commented Jul 1, 2019

jlebon left a comment

arithx commented Jul 5, 2019

sinnykumari commented Jul 10, 2019

sinnykumari commented Jul 10, 2019

jlebon left a comment

jlebon Jul 11, 2019

sinnykumari Jul 11, 2019

jlebon left a comment

sinnykumari commented Jul 11, 2019

sinnykumari commented Jul 11, 2019


		if releaseArch.Media.Azure.Images.Global != nil {
		if releaseArch.Media.Azure.Images != nil &&

Generate stream metadata from releases #1

Generate stream metadata from releases #1

Conversation

sinnykumari commented Jun 28, 2019

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinnykumari Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinnykumari commented Jul 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Jul 1, 2019

jlebon left a comment

Choose a reason for hiding this comment

arithx commented Jul 5, 2019

sinnykumari commented Jul 10, 2019

sinnykumari commented Jul 10, 2019

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

sinnykumari commented Jul 11, 2019

sinnykumari commented Jul 11, 2019

sinnykumari Jul 1, 2019 •

edited

Loading