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

compose: Add rpmostree.rpmmd-repo metadata to commits by default #1079

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

This is a revisit of a PR for client-side layering: #1072
Here though we're doing this by default for server-side composes.
There are a few reasons to do this; first, I'm seeing an issue
in some of our Jenkins jobs for Fedora that hit "mirror roulette"
and end up creating commits that "revert" to older versions temporarily.

While I've certainly pitched this as a feature, I think
we really want something like --force-older-timestamp - basically
error out if the timestamps on one or more input repos were older.
Not doing that in this patch, but it paves the way to do so.

Second, I'd like to use this data in the ostree.source-title
metadata key down the line. Something like:

└ rpmmd: fedora-26 (20170310), fedora-26-updates (20171101)

(This could be a lot nicer if we drive versioning in to the rpm-md repo info,
and e.g. there's some friendly "week number" style versioning for the updates
repo now that it's batched...for now we have timestamps)

For CentOS/RHELAH this gets interesting and potentially more verbose,
to the point where we may want to render it more explicitly.

But anyways, let's do this now, as it will be useful even without
an explicit rendering, since users can do e.g. ostree show on
a base commit hash to dump the data.

I had a concern that some users may not want to emit this metadata;
they can currently do --add-metadata-string rpmostree.rpmmd-repos ''
and that will "win".

This is a revisit of a PR for client-side layering: coreos#1072
Here though we're doing this by default for server-side composes.
There are a few reasons to do this; first, I'm seeing an issue
in some of our Jenkins jobs for Fedora that hit "mirror roulette"
and end up creating commits that "revert" to older versions temporarily.

While I've [certainly pitched](https://lists.fedoraproject.org/archives/list/[email protected]/message/IMPE6KCRBHCEJH5VBE6ZFIRLPAD743JT/) this as a feature, I think
we really want something like `--force-older-timestamp` - basically
error out if the timestamps on one or more input repos were older.
Not doing that in this patch, but it paves the way to do so.

Second, I'd like to use this data in the `ostree.source-title`
metadata key down the line.  Something like:

`└ rpmmd: fedora-26 (20170310), fedora-26-updates (20171101)`

(This could be a lot nicer if we drive versioning in to the rpm-md repo info,
 and e.g. there's some friendly "week number" style versioning for the updates
 repo now that it's batched...for now we have timestamps)

For CentOS/RHELAH this gets interesting and potentially more verbose,
to the point where we may want to render it more explicitly.

But anyways, let's do this now, as it will be useful even without
an explicit rendering, since users can do e.g. `ostree show` on
a base commit hash to dump the data.

I had a concern that some users may not want to emit this metadata;
they can currently do `--add-metadata-string rpmostree.rpmmd-repos ''`
and that will "win".
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.

Makes sense. Just a minor indentation nit, but meh, not worth another round trip.

g_variant_builder_init (&repo_builder, (GVariantType*)"a{sv}");
DnfRepo *repo = repos->pdata[i];
const char *id = dnf_repo_get_id (repo);
g_variant_builder_add (&repo_builder, "{sv}", "id", g_variant_new_string (id));
Copy link
Member

Choose a reason for hiding this comment

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

Minor: indentation off here.

@jlebon
Copy link
Member

jlebon commented Nov 3, 2017

@rh-atomic-bot r+ 1f250a0

@rh-atomic-bot
Copy link

⌛ Testing commit 1f250a0 with merge 4c7f34e...

rh-atomic-bot pushed a commit that referenced this pull request Nov 3, 2017
This is a revisit of a PR for client-side layering: #1072
Here though we're doing this by default for server-side composes.
There are a few reasons to do this; first, I'm seeing an issue
in some of our Jenkins jobs for Fedora that hit "mirror roulette"
and end up creating commits that "revert" to older versions temporarily.

While I've [certainly pitched](https://lists.fedoraproject.org/archives/list/[email protected]/message/IMPE6KCRBHCEJH5VBE6ZFIRLPAD743JT/) this as a feature, I think
we really want something like `--force-older-timestamp` - basically
error out if the timestamps on one or more input repos were older.
Not doing that in this patch, but it paves the way to do so.

Second, I'd like to use this data in the `ostree.source-title`
metadata key down the line.  Something like:

`└ rpmmd: fedora-26 (20170310), fedora-26-updates (20171101)`

(This could be a lot nicer if we drive versioning in to the rpm-md repo info,
 and e.g. there's some friendly "week number" style versioning for the updates
 repo now that it's batched...for now we have timestamps)

For CentOS/RHELAH this gets interesting and potentially more verbose,
to the point where we may want to render it more explicitly.

But anyways, let's do this now, as it will be useful even without
an explicit rendering, since users can do e.g. `ostree show` on
a base commit hash to dump the data.

I had a concern that some users may not want to emit this metadata;
they can currently do `--add-metadata-string rpmostree.rpmmd-repos ''`
and that will "win".

Closes: #1079
Approved by: jlebon
@jlebon
Copy link
Member

jlebon commented Nov 3, 2017

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

💥 Test timed out

@jlebon
Copy link
Member

jlebon commented Nov 4, 2017

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 1f250a0 with merge e7a42f7...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing e7a42f7 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants