Skip to content
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

Add S3 metadata in meta.json #2731

Merged
merged 3 commits into from
Apr 1, 2022
Merged

Add S3 metadata in meta.json #2731

merged 3 commits into from
Apr 1, 2022

Conversation

ravanelli
Copy link
Member

No description provided.

@ravanelli ravanelli changed the title Schema Add S3 in metadata in meta.json Feb 28, 2022
@ravanelli ravanelli changed the title Add S3 in metadata in meta.json Add S3 metadata in meta.json Feb 28, 2022
@miabbott
Copy link
Member

Let's mark this as Draft, as I think there is some more design to be discussed here.

@miabbott
Copy link
Member

I think the primary question is how do we want to encode information that is specific to the build pipeline into meta.json?

Perhaps a new function that we can call during the pipeline execution to write the specific values from the pipeline?

@ravanelli
Copy link
Member Author

ravanelli commented Feb 28, 2022

I think the primary question is how do we want to encode information that is specific to the build pipeline into meta.json?

Perhaps a new function that we can call during the pipeline execution to write the specific values from the pipeline?

We can add some arg for cmd-buildupload (s3 --meta) and use the function created in this commit update_meta_json to update it when we need.

@miabbott
Copy link
Member

We can add some arg for cmd-buildupload (s3 --meta) and use the function created in this commit update_meta_json to update it when we need.

That sounds good to me! Let's keep this in Draft to see if other folks have input.

Could you also edit the first comment to give a bit more rationale for the change and why we want it?

@jlebon
Copy link
Member

jlebon commented Feb 28, 2022

Could you also edit the first comment to give a bit more rationale for the change and why we want it?

👍 That would be welcome.

I'm assuming this is related to GC, correct? It sounds like you already have a design in mind. Would be good to write it up somewhere where we can discuss it (maybe a new issue on this repo?).

@miabbott
Copy link
Member

I'm assuming this is related to GC, correct? It sounds like you already have a design in mind. Would be good to write it up somewhere where we can discuss it (maybe a new issue on this repo?).

It's more about providing information about the source of truth for RHCOS builds see - https://issues.redhat.com/browse/COS-1224

@miabbott
Copy link
Member

miabbott commented Mar 3, 2022

Opened an issue for wider discussion - #2739

@ravanelli ravanelli force-pushed the schema branch 2 times, most recently from 2c3060f to 29accc9 Compare March 7, 2022 19:12
@ravanelli ravanelli marked this pull request as ready for review March 7, 2022 19:14
@ravanelli ravanelli marked this pull request as draft March 7, 2022 19:17
@ravanelli ravanelli force-pushed the schema branch 2 times, most recently from f95592c to 2b48f39 Compare March 7, 2022 19:44
@ravanelli ravanelli marked this pull request as ready for review March 7, 2022 19:47
src/cmd-buildupload Outdated Show resolved Hide resolved
@ravanelli ravanelli force-pushed the schema branch 4 times, most recently from 250df18 to c69f5fe Compare March 11, 2022 19:08
schema/generate-schema.sh Outdated Show resolved Hide resolved
schema/generate-schema.sh Outdated Show resolved Hide resolved
schema/cosa/cosa_v1.go Outdated Show resolved Hide resolved
schema/cosa/cosa_v1.go Outdated Show resolved Hide resolved
@ravanelli ravanelli force-pushed the schema branch 2 times, most recently from 9e6e6f1 to 56de10b Compare March 16, 2022 15:05
@ravanelli ravanelli requested a review from jlebon March 16, 2022 15:06
Copy link
Member

@miabbott miabbott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small change requested, but otherwise i think this looks ready!

@miabbott
Copy link
Member

is there an example meta.json with the new information recorded in it that we can see?

jlebon
jlebon previously approved these changes Mar 24, 2022
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM overall! Just wanted to make sure also that you saw #2731 (comment) and #2731 (comment).

@ravanelli
Copy link
Member Author

This LGTM overall! Just wanted to make sure also that you saw #2731 (comment) and #2731 (comment).

Yeah, totally missed these. Thanks for all the review @jlebon

help='Store key information in meta.json')

upload_cmd.add_argument(
'--s3-url', required=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change the parameter name to public-url too? I was thinking in let it as url. And only use the public-url structure .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to be consistent, but personally fine with it. I'm not really concerned about cosa koji-upload becoming some API that people outside the CoreOS team starts using.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer it to be consistent, but won't block merging on it for the same reasons Jonathan says. :)

src/v1.json Outdated Show resolved Hide resolved
 - Add S3 metadata in meta.json

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
-  Update generate-schema to reflex changes done
in golang binaries and directory tree

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
@ravanelli ravanelli force-pushed the schema branch 3 times, most recently from d044f20 to 4e6e43f Compare March 30, 2022 14:57
 - This change is needed due brew/compliance improvements where we need
to keep more information about where the artifacts are archived;
 - Add s3 parameters in order to update meta.json with
bucket, prefix and url information used in S3 upload.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
help='Store key information in meta.json')

upload_cmd.add_argument(
'--s3-url', required=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to be consistent, but personally fine with it. I'm not really concerned about cosa koji-upload becoming some API that people outside the CoreOS team starts using.

@ravanelli
Copy link
Member Author

/retest-required

@cgwalters cgwalters merged commit 253f9bb into coreos:main Apr 1, 2022
@ravanelli ravanelli deleted the schema branch December 5, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants