1470232 - improve testing for version bump task as it's a leaf node

Status: RESOLVED WONTFIX

(Release Engineering :: Release Automation: Other, defect)

Description

[Mihai Tabara [:mtabara]⌚️GMT]

Follow-up API / in-tree check to ensure that the in-tree value now asserts the expected version passed in the payload.

Comment 1

[Simon Fraser [:sfraser] ⌚️GMT]

https://github.com/mozilla-releng/treescript/pull/24

Comment 2

[Simon Fraser [:sfraser] ⌚️GMT]

Currently working on a script to examine the pushlog for recent version bump changes.

Comment 3

[Simon Fraser [:sfraser] ⌚️GMT]

Attachment: verify_version_bump.py

As a concept, rather than code review, what's your opinion? 

I'm thinking if we have a task that runs the following, then this should catch most error cases outside treescript:

./mach python ./testing/mozharness/scripts/release/verify_version_bump.py --since-revision ${GECKO_HEAD_REVISION}

Comment 4

[Justin Wood (:Callek)]

I'm less sure on this one, in particular because it has mozilla-release hardcoded and because it does a check unbounded starting at HEAD.

I'd like to maybe incorporate the tagging of BuildN we did, so that it checks between buildN and HEAD for a version bump, and noteworthy of course is that with the code as-is, a ver bump on a branch (that doesn't share an ancestor with our buildN) will still trigger this as ok.

Since we have hg locally we could do (against our local hg clone) something like:

$ hg log -q -r "(FIREFOX_62_0b5_BUILD1:beta and descendants(FIREFOX_62_0b5_BUILD1))" browser/config/version_display.txt browser/config/version.txt

where `beta` would be the head rev. This uses `:` which basically means `anything pushed from the place the BUILDN tag is and our head` and then descendants ensures we get only things that are offshoot from buildN

Comment 5

[Simon Fraser [:sfraser] ⌚️GMT]

(In reply to Justin Wood (:Callek) from comment #4)
> I'm less sure on this one, in particular because it has mozilla-release
> hardcoded and because it does a check unbounded starting at HEAD.

This is why I asked about the concept, not for a code review :) The current draft I have doesn't have it hard-coded. So, are you, in principle, ok with the idea of a script like this existing, and being run a after the version bump task?

> I'd like to maybe incorporate the tagging of BuildN we did, so that it
> checks between buildN and HEAD for a version bump, 

What does this give us that's not covered by passing in the build revision?

> and noteworthy of course
> is that with the code as-is, a ver bump on a branch (that doesn't share an
> ancestor with our buildN) will still trigger this as ok.

I'm not sure I'm following. This is looking for a change to those files in a given repo, regardless of ancestry.
 
> Since we have hg locally we could do (against our local hg clone) something
> like:

We discussed that this would be best in a separate task, so we don't have a clone locally at the point this would be run. We could add it, but it's heavyweight.
 
> $ hg log -q -r "(FIREFOX_62_0b5_BUILD1:beta and
> descendants(FIREFOX_62_0b5_BUILD1))" browser/config/version_display.txt
> browser/config/version.txt
> 
> where `beta` would be the head rev. This uses `:` which basically means
> `anything pushed from the place the BUILDN tag is and our head` and then
> descendants ensures we get only things that are offshoot from buildN

If we're ok with doing a clone just to run this command, then sure. If we're doing that then the script probably shouldn't live inside mozharness as it does for now.

Comment 6

[Justin Wood (:Callek)]

(In reply to Simon Fraser [:sfraser] ⌚️GMT from comment #5)
> (In reply to Justin Wood (:Callek) from comment #4)
> > I'm less sure on this one, in particular because it has mozilla-release
> > hardcoded and because it does a check unbounded starting at HEAD.
> 
> This is why I asked about the concept, not for a code review :) The current
> draft I have doesn't have it hard-coded. So, are you, in principle, ok with
> the idea of a script like this existing, and being run a after the version
> bump task?

Yes, I'm ok with it, I'm not certain the talk in channel the other day felt this was even strictly necessary but I have no objections.

> > I'd like to maybe incorporate the tagging of BuildN we did, so that it
> > checks between buildN and HEAD for a version bump, 
> 
> What does this give us that's not covered by passing in the build revision?

if we build on rev B then TB builds on rev C and ver bumps with rev D and we never ver bumped (E was pushed by a dev) we could think the TB bump was ours.

A-B-E
   \-C-D

> > and noteworthy of course
> > is that with the code as-is, a ver bump on a branch (that doesn't share an
> > ancestor with our buildN) will still trigger this as ok.
> 
> I'm not sure I'm following. This is looking for a change to those files in a
> given repo, regardless of ancestry.

This hg command would make sure that we don't find a ver bump like this (C is our buildN tag) where a ver bump happened on E, though wasn't our release (our release would look at C, F, G, H) and we might have ver bumped with H but the HEAD of beta might be F.

A - B - C - F
 |   \- G - H
 \- D - E

> > Since we have hg locally we could do (against our local hg clone) something
> > like:
> 
> We discussed that this would be best in a separate task, so we don't have a
> clone locally at the point this would be run. We could add it, but it's
> heavyweight.

I think we're both confused then. I'm imagining this running as a seperate-from-treescript task, which clones hg, (ala: run-task) and then runs mozharness from that hg clone. This would mean we have an hg clone locally.

Are you thinking differently?

> > $ hg log -q -r "(FIREFOX_62_0b5_BUILD1:beta and
> > descendants(FIREFOX_62_0b5_BUILD1))" browser/config/version_display.txt
> > browser/config/version.txt
> > 
> > where `beta` would be the head rev. This uses `:` which basically means
> > `anything pushed from the place the BUILDN tag is and our head` and then
> > descendants ensures we get only things that are offshoot from buildN
> 
> If we're ok with doing a clone just to run this command, then sure. If we're
> doing that then the script probably shouldn't live inside mozharness as it
> does for now.

If you're thinking differently (no hg clone at all) then I agree, no sense in using hg commands directly (more overhead), but if we already have a clone due to this then using hg commands is probably better.

Comment 7

[Simon Fraser [:sfraser] ⌚️GMT]

(In reply to Justin Wood (:Callek) from comment #6)
> (In reply to Simon Fraser [:sfraser] ⌚️GMT from comment #5)
> > (In reply to Justin Wood (:Callek) from comment #4)
> > > I'm less sure on this one, in particular because it has mozilla-release
> > > hardcoded and because it does a check unbounded starting at HEAD.
> > 
> > This is why I asked about the concept, not for a code review :) The current
> > draft I have doesn't have it hard-coded. So, are you, in principle, ok with
> > the idea of a script like this existing, and being run a after the version
> > bump task?
> 
> Yes, I'm ok with it, I'm not certain the talk in channel the other day felt
> this was even strictly necessary but I have no objections.

"We don't trust external services" is the core part of the request that we've been given, for this sort of graph testing. I don't think I managed to get that across, last week, as people thought 'hg push' could just be trusted to do the right thing.

> > > I'd like to maybe incorporate the tagging of BuildN we did, so that it
> > > checks between buildN and HEAD for a version bump, 
> > 
> > What does this give us that's not covered by passing in the build revision?
> 
> if we build on rev B then TB builds on rev C and ver bumps with rev D and we
> never ver bumped (E was pushed by a dev) we could think the TB bump was ours.
> 
> A-B-E
>    \-C-D

Thunderbird builds from mozilla-beta, mozilla-release and mozilla-esr60? I thought they had comm-* repositories?

https://hg.mozilla.org/mozilla-central/pushlog/ only appears to have mozilla-central pushes in it, similarly for the others. What am I missing?

> > We discussed that this would be best in a separate task, so we don't have a
> > clone locally at the point this would be run. We could add it, but it's
> > heavyweight.
> 
> I think we're both confused then. I'm imagining this running as a
> seperate-from-treescript task, which clones hg, (ala: run-task) and then
> runs mozharness from that hg clone. This would mean we have an hg clone
> locally.
> 
> Are you thinking differently?

The impression I got was that if it was going to have 'clone a repo for a few minutes, just to tick the box' then it should just be in treescript, as it would take less time. It does seem remarkably heavyweight to do a clone just for one 'hg log' command, although I do see that using the tags makes the query cleaner (although the hg command itself, not so much)
 
> > > $ hg log -q -r "(FIREFOX_62_0b5_BUILD1:beta and
> > > descendants(FIREFOX_62_0b5_BUILD1))" browser/config/version_display.txt
> > > browser/config/version.txt
> > > 
> > > where `beta` would be the head rev. This uses `:` which basically means
> > > `anything pushed from the place the BUILDN tag is and our head` and then
> > > descendants ensures we get only things that are offshoot from buildN

Why is 'beta' the head revision here? Or is that just the example of using mozilla-beta?

Comment 8

[Simon Fraser [:sfraser] ⌚️GMT]

I think that due to disagreements over whether this is required or not, and the apparent complexity involved in making sure it's safe against the various odd branching/merging practices in use, it's probably not worth the effort at this stage.