Bug #9740

Tails pythonlib doesn't clean branch_name enough to fit job's name

Added by bertagaz 2015-07-14 05:19:41 . Updated 2015-08-26 06:11:18 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Continuous Integration
Target version:
Tails_1.5
Start date:
2015-07-14
Due date:
% Done:

100%

Feature Branch:
Type of work:
Code
Blueprint:

Starter:
0
Affected tool:
Deliverable for:
266

Description

While deploying Feature #8658, a branch in the Tails git repo had the + character in its name, resulting in Jenkins poorly handling the automatically created job.

The Tails pythonlib had to be patched (see b4032d07) in emergency to get the automated builds deployed, but this proved that this part of its code is clearly not reliable enough.

We should probably document in contribute/git that branches should only contain characters in the [a-zA-Z0-9-_]+ range, but also implement a more elegant whitelist based method for the branch_name cleaning code.

Subtasks

History

#1 Updated by bertagaz 2015-07-14 05:20:06

#2 Updated by bertagaz 2015-07-14 05:55:34

  • Status changed from Confirmed to In Progress
  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 0 to 80
  • QA Check changed from Dev Needed to Ready for QA

Updated the Tails pythonlib with a better method to clean the branch_name, and deployed it. Now it’s whitelisted to the [a-zA-Z0-9-_.] characters. See commits b5959cd and a6c3a0b. You may find better ways to write that I suspect. :)

#3 Updated by intrigeri 2015-07-16 02:57:14

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

Good catch! Great that you deployed something that fixes the practical problem at hand. Let’s now discuss implementation details :)

> Updated the Tails pythonlib with a better method to clean the branch_name, and deployed it. Now it’s whitelisted to the [a-zA-Z0-9-_.] characters.

Why not reuse the same transformation we use for a similar need (branch to APT suite name) in the ref_name_to_suite() function (files/reprepro/functions.sh in puppet-tails)?

        for char in branch:
            if not re.search(job_name_match_re, char):
                branch = branch.replace(char, '-')

This seems like a very “creative” (read: weird and unefficient) way to replace all chars, except those in a whitelist, with a dash. See how we do it in the aforementioned example, using ^ to match any char that’s not in the whitelist => this way you can replace these 3 lines with a single one, and avoid iterating over each char + calling replace for every char that’s not on the whitelist.

#4 Updated by intrigeri 2015-07-16 03:09:22

#5 Updated by intrigeri 2015-07-16 03:09:36

#6 Updated by intrigeri 2015-07-16 03:09:51

(IMO that’s part of phase one, since without this, all active branches cannot be built.)

#7 Updated by intrigeri 2015-07-16 03:10:14

  • blocks #8668 added

#8 Updated by bertagaz 2015-07-23 02:56:38

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Good catch! Great that you deployed something that fixes the practical problem at hand. Let’s now discuss implementation details :)
>
> Why not reuse the same transformation we use for a similar need (branch to APT suite name) in the ref_name_to_suite() function (files/reprepro/functions.sh in puppet-tails)?

Because I forgot to check it.

> This seems like a very “creative” (read: weird and unefficient) way to replace all chars, except those in a whitelist, with a dash. See how we do it in the aforementioned example, using ^ to match any char that’s not in the whitelist => this way you can replace these 3 lines with a single one, and avoid iterating over each char + calling replace for every char that’s not on the whitelist.

Yeah regexp are really not my thing, so I just used this odd implementation.

Thanks to your guidelines, I believe it’s now fixed.

#9 Updated by intrigeri 2015-07-29 00:50:40

  • Status changed from In Progress to Resolved
  • Assignee deleted (intrigeri)
  • % Done changed from 80 to 100
  • QA Check changed from Ready for QA to Pass

Good!

#10 Updated by intrigeri 2015-08-26 06:11:18

  • Deliverable for set to 266