[tcmigration] make version bump builder idempotent for tagging/version bumping

RESOLVED WONTFIX

Status

enhancement
P2
normal
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: mtabara, Assigned: geekodour08, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [releaseduty])

Attachments

(1 attachment, 1 obsolete attachment)

Now that we also have Fennec bumping version within TC relpro, we need to make sure Firefox/Fennec don't step on each others toes. For this reason, we came up with a race-condition mini-condition in bug 1344229, more specifically[1]. But this is not all. We also need to make the tagging part idempotent so that we can rerun this script without the fear of retagging.

We could do this by taking that initial list of tags[2] and see if it's already generated in the list of tags. For that we need to double-check the returning code of the `self.hg_tag` method if any, and correlate it.

[1]: https://hg.mozilla.org/releases/mozilla-beta/rev/c32166f8c0c4
[2]: https://hg.mozilla.org/releases/mozilla-beta/file/tip/testing/mozharness/scripts/release/postrelease_version_bump.py#l175
Mentor: rail, mtabara
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [lang=python]
Posted patch fixes1346465.patch (obsolete) — Splinter Review
This is a proposal for the fix, but I have a few questions.
1. What should be done in postrelease_version_bump.py once you know it's a duplicate.
2. The self.hg_tag returns the return of self.run_command in repo_manipulation.py, but I was unable to track where self.run_command exists,
So I wrote some dummy code in postrelease_version_bump.py for the check.

Let me know I am going in the right way and what modifications should I make.
Flags: needinfo?(rail)
Thanks for your help!

(In reply to Hrishikesh Barman[:geekodour08] from comment #1)
> Created attachment 8851297 [details] [diff] [review]
> fixes1346465.patch
> 
> This is a proposal for the fix, but I have a few questions.
> 1. What should be done in postrelease_version_bump.py once you know it's a
> duplicate.

See my comments below.

> 2. The self.hg_tag returns the return of self.run_command in
> repo_manipulation.py, but I was unable to track where self.run_command
> exists,

It's just a nice wrapper around subprocess.Popen, see https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#1266

> So I wrote some dummy code in postrelease_version_bump.py for the check.
> 
> Let me know I am going in the right way and what modifications should I make.


The idea is to make sure that the revision that we pass to this script corresponds to the tag(s) we want to use.

You may have some cases:

1) The tag doesn't exist
2) the tag does exist, but points to a different revision
3) the tag does exist and points to the given revision

Case 3) is the easiest one: just skip it. For 1) and 2) you can probably use the same command to tag (case 2) would require --force though.

You can use something like `hg id --revision $tag --id` (which shows what revision is associated with the given tag) to detect the cases above.

Also it would be great to have some tests. ;)
Flags: needinfo?(rail)
I tried implementing checks for the three cases you mentioned in repo_manipulation.py, also in
scripts/release/postrelease_version_bump::PostReleaseVersionBump::tag function, 
the hg_tag function is called with Force=True, so forcing it for case 2 seems redundant. 

Please review it and let me know.

Also, can you explain a bit more on hg id --revision $tag --id , I did not really get how to put that information to use. 

I'll write the tests, but It will be really helpful if you can tell me where to write the tests, I checked the test folders but am not sure where to write them. Secondly, what are the tests for? for "revision that we pass to this script corresponds to the tag(s)" ?
Attachment #8851297 - Attachment is obsolete: true
Flags: needinfo?(rail)
No longer blocks: 1346892
(In reply to Hrishikesh Barman[:geekodour08] from comment #3)
> Also, can you explain a bit more on hg id --revision $tag --id , I did not
> really get how to put that information to use. 

Let's say you want to tag a repo using revision $rev and tag $tag. Depending on the state of the repo you may have 4 possible scenarios:

1) No such revision. You can test this by running
  hg id --rev $rev
It exist non zero in case you don't have this revision in the repo

2) No such tag (yet). This is what happens when you tag first time. You can check this by running
   hg id --rev $tag
It exist non zero in case you don't have this tag in the repo

3) The tag already exists and points to the same revision. To identify what revision a particular tag points, you run 
  hg id --rev $tag --id
which prints the revision only (--id). You can compare the output to the 

4) The tag already exists, but points to a different revision. Similar to 3) 
  hg id --rev $tag --id
will print the actual revision, that you would need to compare the desired one to.

Also, keep in mind that revisions can be short and long. You can use something like
  hg log -r $rev --template '{node}\n'
to print the long changeset ID.

Mihai, can you look at the patch, whenever you have time please.
Flags: needinfo?(rail) → needinfo?(mtabara)
Thanks for the information, I'll see If I can make more changes to it and report back here.
Sorry for delays, been out PTo & traveling last week.
Will glance at this patch tomorrow morning.
Leaving the NI here until I reply back. Thanks!
Hello. It seems Mihal hasn't got to this patch review as yet. Is it alright if Rail reviews it and gives feedback? I'd like to help finish this off if I can.
Flags: needinfo?(rail)
Sorry about that. There is a way to explicitly ask for a review by setting the r? flag on your attachment. Let me do that.
Flags: needinfo?(rail)
Attachment #8851405 - Flags: review?(mtabara)
(In reply to Leni Kadali from comment #7)
> Hello. It seems Mihal hasn't got to this patch review as yet. Is it alright
> if Rail reviews it and gives feedback? I'd like to help finish this off if I
> can.

Really sorry for this, somehow it slipped my radar :( 
Clearing the NI request, leaving the r? and looking at this patch this week.
Flags: needinfo?(mtabara)
status update: patch looks good overall, forgot to mention this last week. I want to take a moment though to test this as this is a sensitive operation that we need to make sure we do right.

Apologies again for delays in answering. This is my fault and wasn't supposed to take that long.
See Also: → 1301782
Fennec is now finally re-integrated within Ship-it so we can now take care of remaining leftovers: version bump (current bug), source builder and checksums. This bug should have been tested and fixed long time ago, almost 6 months. I apologize for all the delays. Let me see how I can test those ...
Assignee: nobody → hrishikeshbman
Depends on: 1347635
Priority: P3 → P2
Whiteboard: [lang=python] → [lang=python][releaseduty]
Whiteboard: [lang=python][releaseduty] → [releaseduty]
Blocks: 1402084
Comment on attachment 8851405 [details] [diff] [review]
fixes1346465.patch

removing the r? flag to remove from my queue.
Attachment #8851405 - Flags: review?(mtabara)
So we now have Fennec checksums + source \o/
This is the last task standing. Should we address the idempotency too?
:rail - should I try to test the patch above or do we want another workaround?
Flags: needinfo?(rail)
I'm not sure if the patch actually works. The condition `if set(cmd).issuperset(set(tags)) and revision in cmd:` doesn't make too much sense to me (cmd = ['hg', 'tag'] and tags being a list of tags). There has been no activity in this bug for a while. I think we can start fresh. :)
Flags: needinfo?(rail)
(In reply to Rail Aliiev [:rail] ⌚️ET from comment #14)
> I'm not sure if the patch actually works. The condition `if
> set(cmd).issuperset(set(tags)) and revision in cmd:` doesn't make too much
> sense to me (cmd = ['hg', 'tag'] and tags being a list of tags). There has
> been no activity in this bug for a while. I think we can start fresh. :)

Okay, let's close this then and add this as a requirement in the tcmigration world, cleanup/manana section.

@hrishikeshbman@gmail.com:
Despite we're not taking this into production now, thank you for your contribution. The work in this bug would be the groundwork for our follow-up approach. Thank you!
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.