Feature #8657

Write library code that implements the branch to build parameters mapping

Added by intrigeri 2015-01-09 17:44:47 . Updated 2015-08-05 09:30:26 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Continuous Integration
Target version:
Tails_1.4.1
Start date:
2015-01-09
Due date:
2015-07-15
% Done:

100%

Feature Branch:
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:
266

Description

(Using the results of the Feature #8655 discussion.)

Files

list_tails_unmerged_branches (1723 B) bertagaz, 2015-02-02 22:49:43
list_tails_unmerged_branches (2777 B) bertagaz, 2015-02-05 14:54:09

Subtasks

Related issues

Blocked by Tails - Feature #8655: Lead the discussion to specify what kind of branch needs to be automatically built, and when Resolved 2015-01-09 2015-06-01
Blocks Tails - Feature #8656: Write code that generates a set of Jenkins jobs for all branches we want to automatically build Resolved 2015-01-09 2015-07-15

History

#1 Updated by intrigeri 2015-01-09 17:45:22

  • blocked by Feature #8655: Lead the discussion to specify what kind of branch needs to be automatically built, and when added

#2 Updated by intrigeri 2015-01-09 17:45:47

  • blocks Feature #8656: Write code that generates a set of Jenkins jobs for all branches we want to automatically build added

#3 Updated by bertagaz 2015-01-10 06:28:05

  • Target version changed from Tails_1.4 to Tails_1.3

#4 Updated by bertagaz 2015-01-10 06:28:44

  • Target version changed from Tails_1.3 to Tails_1.3.2

#5 Updated by bertagaz 2015-01-10 17:09:52

  • Target version changed from Tails_1.3.2 to Tails_1.3

#6 Updated by intrigeri 2015-01-11 22:46:00

Whether a branch is merged or not into one of our base branches might be one of the heuristics we want to use.

bertagaz has a script that lists the branches that were not merged into a specified branch. Building upon that, something like this should build a list of the branches that are not merged in any of the target branches. Untested, but happy to give a hand if someone prefers taking this code than rewriting it in their preferred programming language. It could probably be rewritten with basic set operations, for those who are more versed into maths.

my %unmerged_count;
my %unmerged_in;

my @target_branches = ('stable', 'testing', 'devel');

sub branches_not_merged_in {
    my $target_branch = shift;

    # XXX: use bertagaz' script

    return \@unmerged_branches;
}

for my $target_branch (@target_branches) {
    $unmerged_in{$target_branch} = branches_not_merged_in($target_branch);
}

for my $target_branch (@target_branches) {
    for my $topic_branch (@{$unmerged_in{$target_branch}}) {
        $unmerged_count{$topic_branch}++;
    }
}

my @not_merged_anywhere = grep {
    $unmerged_count{$_} == @target_branches
} keys(%unmerged_count);

#7 Updated by bertagaz 2015-02-02 22:50:19

Here’s a first POC in bash that lists the branches unmerged in stable, devel and testing since the last release.
I’ve put some of its logic in functions.

#8 Updated by bertagaz 2015-02-05 14:57:11

An updated version of the script, which had a bug: the output was including branches that has been merged in some of the base branches.

Now, it collects the branches not merged since the last release in all the base branches, then keep those who have not been merged in all of the base branches.

The code should be easier to turn into a library too if anyone wants to do so.

#9 Updated by Dr_Whax 2015-02-10 20:44:19

I’ll play with it a little bit once we got everything building!

#10 Updated by Dr_Whax 2015-02-24 23:29:21

  • Target version changed from Tails_1.3 to Tails_1.3.2

I hope to work on this a bit, this week. Postponing to 1.3.1.

#11 Updated by Dr_Whax 2015-03-13 14:07:58

  • Target version changed from Tails_1.3.2 to Tails_1.4

#12 Updated by bertagaz 2015-03-14 15:00:11

I’d be to keep this ticket flagged for 1.3.1 for now. As we said this was more an indicative due date than a strong one. We’ll be able to delay it for 1.4 (the release where we’ll deploy the automated builds) when 1.3.1 is out.

Seeing it targeting 1.4 now doesn’t reassure me, I’m afraid you’ll loose it from your radar.

Keep in mind that 1.3.1 is due in 24 days, won’t you have some time in that delay to spend some on this issue?

#13 Updated by Dr_Whax 2015-03-21 12:30:41

  • Status changed from Confirmed to In Progress

#14 Updated by Dr_Whax 2015-03-21 12:32:18

bertagaz wrote:
> I’d be to keep this ticket flagged for 1.3.1 for now. As we said this was more an indicative due date than a strong one. We’ll be able to delay it for 1.4 (the release where we’ll deploy the automated builds) when 1.3.1 is out.

Fair enough.

>
> Seeing it targeting 1.4 now doesn’t reassure me, I’m afraid you’ll loose it from your radar.

It isn’t off my radar, but whether I can make it happen is a different story. Lots of things going on :/

> Keep in mind that 1.3.1 is due in 24 days, won’t you have some time in that delay to spend some on this issue?

Off and on, yes! :)

#15 Updated by Dr_Whax 2015-03-21 15:48:28

Define input/output of the library code to bind branches to jobs :)

Input:
- Full checkout of Tails.git repository.

- Date of the previous release.
- List of unmerged topic branches into each base branches
- Counting number of base branches
- Listing the pair of unmerged topic branches for each base branches
- With this list, finding the one that is not merged into every base branch

Output:
- List of branches to build, that are base branches + topic branches that are not merged into any base branch and had new commits since the last release. For reach branch, we want this.
- Name of unmerged topic branch with slash
- E-mail of last committer before build
- Name of job from corresponding unmerged topic branch without slashes
- Boolean —> is it a base branch or not.
- Ticket number

#16 Updated by intrigeri 2015-03-29 15:10:46

“E-mail of last committer before build” won’t actually be needed.

job_recipients should be among release_managers and whoever_broke_the_build, to match what the script waiting for this library is using.

#17 Updated by intrigeri 2015-03-29 15:39:53

Dr_Whax wrote:
> It isn’t off my radar, but whether I can make it happen is a different story. Lots of things going on :/

I see no progress on this front since more than a month. When do you think you’ll be able to propose something that’s production-ready? (Please take into account that a few review/improve iterations will probably be needed along the way.)

If the realistic answer is “not within the next 2 weeks”, then I guess we’ll stop waiting and take care of it ourselves.

#18 Updated by bertagaz 2015-04-06 16:15:04

  • Assignee changed from Dr_Whax to bertagaz

Assigning to myself.

#19 Updated by bertagaz 2015-04-06 17:29:49

  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 0 to 20

Pushed commits that fill the remaining bits of this lib.

Assigning to intrigeri for a first review.

#20 Updated by intrigeri 2015-04-07 09:56:24

  • Assignee changed from intrigeri to bertagaz
  • QA Check set to Dev Needed

Hi!

Good job! I’ve pushed some minor changes on top, please have a good look.

  • What does the -1 option used in committer_date do? I could not find its documentation, and I can’t see how it differs when I add/remove it.
    * I think the is_topic_branch logic is flawed. People use a wider variety of branch name prefixes, they’ll go on inventing new ones, and there’s nothing we can do about it. The commit message doesn’t help me understand why we need that logic at all: if all we want to do is to filter out e.g. origin/HEAD, why not… just do that?
    * Why does all_branches filter out ActiveBranches.ALWAYS_ACTIVE_BRANCHES? This feels totally counter-intuitive, given the method name. And it’s not needed, since active_branches uniquifies the list of branches it returns.
    * lstrip really doesn’t do what you think it does. I trust you it works in the cases where you use it, but that’s by chance. You’ll need to use regexps instead.
    * Please move the job_name computation to a dedicated method. I’d like job_params to stay as simple and short as possible, and if we start adding logics in it now, I’m afraid it’ll go on and on.
    * Having to strip origin/ in compute is a code smell that indicates improper design. It means that active_branches doesn’t return what we want, and it turn it means that all_branches doesn’t either. I suggest passing around already cleaned up branch names only, and re-adding the origin/ prefix on the spot whenever needed (possibly with a helper method if you’re doing that in more than 2 places).
    * I suggest renaming the base_branch job parameter to is_base_branch, otherwise one may believe it contains, well, the name of the base branch :)
    * Having to newrev.lstrip() in rev_list_in_range is another code smell that indicates another design problem. That method should receive a Git revision, not “some string that may contain a Git revision once we strip leading whitespace”. Please fix the root cause, not the symptom — thanks!
    * The regexp for ticket_number feels a bit weak. Perhaps we can specify that the ticket number MUST come immediately after a slash?
    * Assigning the return value of re.search, in ticket_number, to a variable called ticket, is incorrect. As the next lines show, that variable doesn’t contain a ticket number. Please find a better name for it :)

I’ve optimized things a bit, by the way:

  • before any optimization: 37.22s user 4.05s system 84% cpu 48.606 total
  • after my optimizations: 7.92s user 2.00s system 69% cpu 14.289 total

#21 Updated by intrigeri 2015-04-07 10:32:05

  • Assignee changed from bertagaz to intrigeri

Hold on, I’m still working on it.

#22 Updated by intrigeri 2015-04-07 11:10:41

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 20 to 30

Releasing the lock, I’m done for today. I’ve fixed a few of the aforementioned issues (not all, though) and refactored all Git-related bit that weren’t specific to Jenkins into a dedicated class. Please take a look.

#23 Updated by bertagaz 2015-04-07 13:17:28

  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 30 to 20

intrigeri wrote:
> Hi!
>
> Good job! I’ve pushed some minor changes on top, please have a good look.

You worked on most of this items, I think I fixed the other issues you left. Please have a look.

> * What does the -1 option used in committer_date do? I could not find its documentation, and I can’t see how it differs when I add/remove it.

It ensures that when you do that ‘git show …’ command on a tag, you don’t get the additionnal infos it prints for a tag compared to what it prints for a commit, but really just the date.

> I’ve optimized things a bit, by the way:
>
> * before any optimization: 37.22s user 4.05s system 84% cpu 48.606 total
> * after my optimizations: 7.92s user 2.00s system 69% cpu 14.289 total

Hurray! Nice catch, I was thinking of optimization as one of the next step.

> I’ve fixed a few of the aforementioned issues (not all, though) and refactored all Git-related bit that weren’t specific to Jenkins into a dedicated class. Please take a look.

Was sure you were going to play with the Git class, as you said, and I thought it was the right time to abstract that. Was on my schedule after your first review, hope you had fun doing so. It’s much nicer now and appart from an old typo, didn’t find much to say.

#24 Updated by bertagaz 2015-04-07 13:18:07

  • % Done changed from 20 to 30

Fix mess up with percentage.

#25 Updated by intrigeri 2015-04-07 15:07:14

redmine@labs.riseup.net wrote (07 Apr 2015 13:17:29 GMT) :
> Was sure you were going to play with the Git class, as you said, and I thought it was
> the right time to abstract that. Was on my schedule after your first review, hope you
> had fun doing so.

I had! :)

#26 Updated by intrigeri 2015-04-09 08:40:51

  • Assignee changed from intrigeri to bertagaz

Not sure why this was reassigned to me, let’s fix that.

Anyway, while starting working on Feature #7221, I’ve found another problem. The code (perhaps only all_branches) assumes that it’s run with a Git repo that has only one remote configured, and that this remote is called origin. As part of that, all_branches passes the --remotes option to git branch. I see two problems with that:

  • It may be the case wherever we run the job creation script by cron, but in this use case, I think we’ll want to simply use the bare repo we already have, so no need to bother with --remotes and filtering out the origin/ prefix, since our bare repo already has all remote branches configured locally. That’s why e.g. tails-suites-list only has to run git branch, and doesn’t need to pass --remotes nor filter out origin/.
  • It’s not very realistic to satisfy these pre-conditions on one developer’s usual Git clone, so in this use case it’ll require some preparatory work; once we have this in mind, the preparatory work could as well be git clone --reference ~/my/usual/tails/git/.git OFFICIAL_REPO.

So, I think that the Git class should work by default on bare repos, use local branches, and avoid assuming anything about remotes. If we agree on this, then:

  • A little bit of code can be ditched.
  • We’ll need a check for “is that directory a bare repo?” in the class constructor.
  • This will ease the work on Feature #7221 a lot.

What do you think?

#27 Updated by bertagaz 2015-04-10 10:53:08

intrigeri wrote:
> Not sure why this was reassigned to me, let’s fix that.

Because I implemented what was left to do from your first review (note 20), as well as answered to one remaining question in note 23 (–1 option to git show). So we were ate the next review step.

> Anyway, while starting working on Feature #7221, I’ve found another problem. The code (perhaps only all_branches) assumes that it’s run with a Git repo that has only one remote configured, and that this remote is called origin. As part of that, all_branches passes the --remotes option to git branch. I see two problems with that:
>
> * It may be the case wherever we run the job creation script by cron, but in this use case, I think we’ll want to simply use the bare repo we already have, so no need to bother with --remotes and filtering out the origin/ prefix, since our bare repo already has all remote branches configured locally. That’s why e.g. tails-suites-list only has to run git branch, and doesn’t need to pass --remotes nor filter out origin/.
>
> * It’s not very realistic to satisfy these pre-conditions on one developer’s usual Git clone, so in this use case it’ll require some preparatory work; once we have this in mind, the preparatory work could as well be git clone --reference ~/my/usual/tails/git/.git OFFICIAL_REPO.
>
> So, I think that the Git class should work by default on bare repos, use local branches, and avoid assuming anything about remotes. If we agree on this, then:
>
> * A little bit of code can be ditched.
> * We’ll need a check for “is that directory a bare repo?” in the class constructor.
> * This will ease the work on Feature #7221 a lot.
>
> What do you think?

I used the —remote option because I thought that in other use cases (like devs using this lib on their own repo) we might not want not to depend on local branches not yet pushed, or other troubles of that kind, but use the origin remote (which I think always exists, it’s created when you clone a repo, —bare or not) which is the canonical reference place for the Tails git repo.

Also the output was more difficult to parse, as it contains this star in front of the checked out branch.

But if it adds difficulties to work on Feature #7221, maybe you’re right and we should use a bare repo (maybe as an optional parameter to the Git class? Might bring too much complexity to the code though).

When you mention “the bare repo we already have”, are you mentionning the puppet-git one? Haven’t think about using it directly, as I assumed we’ll run this cron script inside our jenkins master VM.

Sorry, I’m exhausted and have a bit of hard time following the rationale at the moment. I won’t have time to work on that before next week, feel free to go on with that if it’s too late.

#28 Updated by intrigeri 2015-04-16 08:59:32

> But if it adds difficulties to work on Feature #7221, maybe you’re right and we should use a bare repo

Yes, please. I suspect it’ll actually make the code simpler, rather than more complicated.

> (maybe as an optional parameter to the Git class? Might bring too much complexity to the code though).

Let’s postpone this to whenever we need to manage non-bare repos with this class.

> When you mention “the bare repo we already have”,

… I was totally confused, sorry.

#29 Updated by bertagaz 2015-04-16 12:24:20

  • Assignee changed from bertagaz to intrigeri

#30 Updated by intrigeri 2015-04-17 17:23:45

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 30 to 50
  • QA Check deleted (Dev Needed)

#32 Updated by intrigeri 2015-05-09 02:06:19

  • Target version changed from Tails_1.4 to Tails_1.4.1

Postponing.

#33 Updated by intrigeri 2015-05-29 11:37:00

  • blocks #8668 added

#34 Updated by bertagaz 2015-06-18 12:10:33

  • Status changed from In Progress to Resolved
  • Assignee deleted (bertagaz)
  • % Done changed from 50 to 100

The pythonlib code has been put in a stable and satisfying state enough to put it in production.

#35 Updated by sajolida 2015-08-05 09:30:26

  • Due date set to 2015-07-15