canonical-ubuntu-qa team mailing list archive

[Paride Legovini <mp+444059@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Nice job! I have some minor review comments, but it basically looks good.

You wrote "Not sure this will work." You have access to venonat, right? Can you please manually test the builder script there?


Diff comments:

> diff --git a/jobs/autopkgtest-cloud/jobs.yaml b/jobs/autopkgtest-cloud/jobs.yaml
> index c71fea4..91a0fb8 100644
> --- a/jobs/autopkgtest-cloud/jobs.yaml
> +++ b/jobs/autopkgtest-cloud/jobs.yaml
> @@ -15,6 +15,38 @@
>  # You should have received a copy of the GNU General Public License along
>  # with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> +- job:
> +    name: daily-charmcraft-pack-check

Prepend autopkgtest-cloud- to the job name for namespacing in Jenkins.

> +    description: |
> +        Runs a daily charmcraft pack for autopkgtest-cloud-worker and
> +        autopkgtest-web charms to check for changes in external
> +        dependencies breaking the charms.
> +    triggers:
> +        - timed: '0 23 * * *'

Let's use a '@daily' timed trigger (Jenkins will randomize execution time).

> +    builders:
> +        - clear-artifacts:
> +        - shell: |
> +            #!/bin/bash

I see you didn't go with `set -e`, that's fine as the script logic explicitly checks for the return code of critical commands (I'm referring to those `|| exit 1`). [To be clear: I am not suggesting to add `set -e` unless you want to adapt all the script logic to it!]

However it may be useful to have `set -x` to make it easier to figure out what happens in Jenkins.

> +            # node needs to be 20.04 i think

What's the part needing 20.04? charmcraft is a snap after all, we shouldn't have this kind of problem.

> +            export http_proxy="http://squid.internal:3128";
> +            export https_proxy="http://squid.internal:3128";
> +            export no_proxy=launchpad.net
> +
> +            sudo snap install charmcraft --classic

Drop the "snap install": we'll install charmcraft once for all of the Jenkins nodes (well, only venonat at the moment).

(In general, let's try not to use 'sudo' commands in Jenkins.)

> +
> +            git clone https://git.launchpad.net/autopkgtest-cloud

I suggest using `git clone --depth 1` to speed up cloning, as we don't need the whole git history.

> +            cd autopkgtest-cloud || exit 1
> +            # charmcraft pack -v -p charms/focal/autopkgtest-cloud-worker/
> +            # charmcraft pack -v -p charms/focal/autopkgtest-web/

I don't get the reason for these two comments. If they're leftover from local testing just drop them.

> +            charmcraft pack --destructive-mode -v -p charms/focal/autopkgtest-cloud-worker/
> +            charmcraft pack --destructive-mode -v -p charms/focal/autopkgtest-web/

Add a `|| exit 1` to these two commands to be sure to exit if they fail but still leave a .charm file behind (as-is the script wouldn't catch this case).

> +            if test -f "autopkgtest-cloud-worker_ubuntu-20.04-amd64.charm" && test -f "autopkgtest-web_ubuntu-20.04-amd64.charm"; then
> +                printf "Charms built successfully"

Add newlines if you use printf, or just use echo if no special formatting is needed.

> +            else
> +                printf "Charms didn't build!"
> +                exit 1
> +            fi
> +
>  
>  - job:
>      name: autopkgtest-cloud-rtd-build-trigger


-- 
https://code.launchpad.net/~andersson123/qa-jenkins-jobs/+git/qa-jenkins-jobs/+merge/444059
Your team Canonical Platform QA Team is subscribed to branch qa-jenkins-jobs:master.