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

tag: new command to manipulate tagging #243

Merged
merged 1 commit into from
Dec 5, 2018
Merged

tag: new command to manipulate tagging #243

merged 1 commit into from
Dec 5, 2018

Conversation

miabbott
Copy link
Member

@miabbott miabbott commented Nov 30, 2018

The build-tag command allows the user to list, create, delete, and
update tags in the builds.json metadata.

@cgwalters
Copy link
Member

Nice! Only skimmed this so far but looks very good. Will have more of a chance to dig in next week.

@cgwalters
Copy link
Member

Did you consider the "git path"? We may have only talked about that internally though hmm.

@miabbott
Copy link
Member Author

miabbott commented Nov 30, 2018

Did you consider the "git path"? We may have only talked about that internally though hmm.

I'm not sure I recall that discussion or what the "git path" is in this context...

@cgwalters
Copy link
Member

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Some example invocations as a comment at the top of the file would be useful.

src/cmd-build-tag Outdated Show resolved Hide resolved
src/cmd-build-tag Outdated Show resolved Hide resolved
src/cmd-build-tag Outdated Show resolved Hide resolved
src/cmd-build-tag Outdated Show resolved Hide resolved
coreos-assembler Outdated Show resolved Hide resolved
src/cmd-build-tag Outdated Show resolved Hide resolved
src/cmd-build-tag Outdated Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Dec 3, 2018

So, the code itself looks sane to me, just had some high-level structuring comments. Though I do wonder about

Did you consider the "git path"? We may have only talked about that internally though hmm.


Basically, it's becoming clearer that the output of coreos-assembler is most useful towards the "buildmaster" stream, and so promotion/smoketest processes need to slurp up the relevant artifacts and put it somewhere else, right? In that sense, I'm not sure if tag management really belongs at the coreos-assembler layer rather than whatever higher level handles promotions.

@jlebon
Copy link
Member

jlebon commented Dec 3, 2018

Ahh right, there's #203 tracking this. Let's discuss there.

@cgwalters
Copy link
Member

Basically, it's becoming clearer that the output of coreos-assembler is most useful towards the "buildmaster" stream, and so promotion/smoketest processes need to slurp up the relevant artifacts and put it somewhere else, right?

Why do you say that?

This approach seems OK to me.

(The reason I was arguing for potentially using git is when one needs to delete data, having git becomes a lot more useful. But that's for GC - representing tags inline as is done here versus actually making them git tags is more of a tossup I feel)

@jlebon
Copy link
Member

jlebon commented Dec 3, 2018

But that's for GC - representing tags inline as is done here versus actually making them git tags is more of a tossup I feel

Ahh OK. To me, those feel pretty connected. One example: how would we tag a build which has already been pruned? E.g. if extended validation of a build takes longer than it takes for the build to be flushed out of builds.json. (Assuming a pipeline that prunes to 3 builds and rebuilds every 30m, that's potentially only 1.5 hours before we can tag the build and snapshot it into a git metadata repo). Or e.g. a "stable" tag that could be many days old if all the builds since then have been broken?

Vs. snapshotting all the build metadata in a git repo and doing tagging there directly. Don't necessarily mean using git tag either, but just decoupling how long a build is in the builds/ dir from when we want to tag it.

@cgwalters
Copy link
Member

E.g. if extended validation of a build takes longer than it takes for the build to be flushed out of builds.json. (Assuming a pipeline that prunes to 3 builds and rebuilds every 30m, that's potentially only 1.5 hours before we can tag the build and snapshot it into a git metadata repo).

At least for RHCOS I don't forsee us pruning anywhere near that aggressively. Pruning would be like "more than two weeks old" or even more.

The default pruning in c-a I think is more for the local dev case.

Storage is cheap.

@jlebon
Copy link
Member

jlebon commented Dec 3, 2018

Storage is cheap.

Yeah, I'm not so much concerned about storage space either but just at the conceptual level:

decoupling how long a build is in the builds/ dir from when we want to tag it.

Anyway, not strongly opposed to this patch. We can always revisit later if we hit any pains.

@miabbott
Copy link
Member Author

miabbott commented Dec 3, 2018

With @jlebon's suggestion of using subparsers, I reworked almost all of the code. So there are a lot of changes compared to the previous version, but I think it is ultimately cleaner and easier to maintain/understand. ⬆️

src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
Copy link
Contributor

@arithx arithx left a comment

Choose a reason for hiding this comment

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

I like list & dict comprehensions 😄

src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
@miabbott
Copy link
Member Author

miabbott commented Dec 4, 2018

New commit with more list/dict comprehension ⬆️

@jlebon
Copy link
Member

jlebon commented Dec 4, 2018

Yeah, I'm not so much concerned about storage space either but just at the conceptual level:

decoupling how long a build is in the builds/ dir from when we want to tag it.

So I was thinking about this some more in the context of the FCOS pipeline and I think the part that was missing for me was that prune needs to learn not to delete builds with tags still pointing at them. Without that, maintaining tags in builds.json is not as useful (or at the very least, it requires implementing pruning at a higher level, which I think is where RHCOS is going, right? But I'd like to stick with prune for FCOS if we can).

And now I see that #184 does mention this as well. So I think this is good, but maybe let's drop the Closes: #184 footer to make sure we don't lose that crucial part?

src/cmd-tag Outdated Show resolved Hide resolved
@jlebon jlebon mentioned this pull request Dec 4, 2018
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
src/cmd-tag Outdated Show resolved Hide resolved
@miabbott
Copy link
Member Author

miabbott commented Dec 5, 2018

So I think this is good, but maybe let's drop the Closes: #184 footer to make sure we don't lose that crucial part?

I didn't interpret the part about enhancing the pruning as an implicit requirement, but I'm fine to drop the Closes from the commit message. It does make sense to enhance prune like that and would be fine as a follow-on.

The `tag` command allows the user to list, create, delete, and
update tags in the `builds.json` metadata.
@miabbott miabbott changed the title build-tag: new command to manipulate tagging tag: new command to manipulate tagging Dec 5, 2018
@miabbott
Copy link
Member Author

miabbott commented Dec 5, 2018

New commit with even more feedback addressed! ⬆️

@cgwalters
Copy link
Member

I played with this locally and it seems to work well. Next step is definitely though to teach prune about this.

@cgwalters
Copy link
Member

Will leave a bit of time for others to comment, if there are no other ones let's merge this in a few hours?

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 5, 2018
Follow-up to coreos#243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
@jlebon
Copy link
Member

jlebon commented Dec 5, 2018

I played with this locally and it seems to work well. Next step is definitely though to teach prune about this.

Done in #245!

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.

LGTM

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 5, 2018
Follow-up to coreos#243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
@miabbott
Copy link
Member Author

miabbott commented Dec 5, 2018

I played with this locally and it seems to work well. Next step is definitely though to teach prune about this.

Done in #245!

That was fast! 🐎 ⚡

@jlebon
Copy link
Member

jlebon commented Dec 5, 2018

OK, merging this! Great job @miabbott putting up with all reviews & tweaks & bikesheds. 🎊

@jlebon jlebon merged commit b89a40a into coreos:master Dec 5, 2018
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 5, 2018
Follow-up to coreos#243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
@miabbott
Copy link
Member Author

miabbott commented Dec 5, 2018

OK, merging this! Great job @miabbott putting up with all reviews & tweaks & bikesheds. confetti_ball

Appreciate all the feedback!

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 5, 2018
Follow-up to coreos#243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 5, 2018
Follow-up to coreos#243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 5, 2018
Follow-up to coreos#243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 5, 2018
Follow-up to coreos#243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
cgwalters pushed a commit that referenced this pull request Dec 6, 2018
Follow-up to #243. Become aware of tags and make sure not to prune
builds that are tagged. I went the path of making tagged builds *not*
counting towards the `--keep=N` limit since that felt the most natural
to me.

One tricky bit is the OSTree repo pruning. The CLI is not really geared
towards this kind of pruning since we now could have older (tagged)
commits that we care about while some newer commits on that same branch
we don't care about. For now, I just swapped it to use
`--keep-younger-than`. Though this does align well with another
improvement I'd like to make to `prune` to learn timestamp-based pruning
instead.
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.

5 participants