Project

General

Profile

Bug #9740

Feature #6090: Automated builds

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

Added by bertagaz almost 4 years ago. Updated over 3 years ago.

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

100%

QA Check:
Pass
Feature Branch:
Type of work:
Code
Blueprint:
Starter:
No
Affected tool:

Description

While deploying #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.

History

#1 Updated by bertagaz almost 4 years ago

  • Parent task set to #9614

#2 Updated by bertagaz almost 4 years ago

  • 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 almost 4 years ago

  • 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 almost 4 years ago

  • Parent task deleted (#9614)

#5 Updated by intrigeri almost 4 years ago

  • Parent task set to #5324

#6 Updated by intrigeri almost 4 years ago

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

#8 Updated by bertagaz almost 4 years ago

  • 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 almost 4 years ago

  • 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!

Also available in: Atom PDF