Skip to content

chore(main): add reusable workflow to test installing slices#119

Merged
cjdcordeiro merged 15 commits intocanonical:mainfrom
rebornplusplus:ci/tests/install-slices/main
Feb 22, 2024
Merged

chore(main): add reusable workflow to test installing slices#119
cjdcordeiro merged 15 commits intocanonical:mainfrom
rebornplusplus:ci/tests/install-slices/main

Conversation

@rebornplusplus
Copy link
Copy Markdown

This PR adds a reusable workflow file .github/workflows/install-slices.yaml which can be used to test installing slices in changed or all slice definition files. It also adds a helper script github/scripts/install-slices used by the mentioned workflow file.


Example: rebornplusplus#2

Rafid Bin Mostofa added 2 commits February 5, 2024 15:44
This commit adds a reusable workflow file
.github/workflows/install-slices.yaml which can be used to test
installing slices in changed or all slice definition files.
@rebornplusplus rebornplusplus self-assigned this Feb 5, 2024
@rebornplusplus rebornplusplus changed the title chore: add reusable workflow to test installing slices chore(main): add reusable workflow to test installing slices Feb 5, 2024
Copy link
Copy Markdown
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit modifies the reusable workflow to be aware if it should test
installing all slices or not, if the install-all is not given. This
reduces complexity on the calling workflow in the ubuntu-* branches.
@rebornplusplus rebornplusplus added the Priority Look at me first label Feb 7, 2024
Copy link
Copy Markdown
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum there's quite a lot of repetition here. You don't need the two separate workflows.

One is enough, where:

on:
  workflow_call:
    inputs:
      install-all:
        description: "If true, install all slices in slices/"
        type: boolean
        default: false
        required: false
  push:
    branches:
      - "main"
    paths:
      - ".github/**"
  pull-request:
    branches:
      - "main"
    paths:
      - ".github/**"
  schedule:
    # Run at 00:00 every day.
    # Ref: https://man7.org/linux/man-pages/man5/crontab.5.html
    - cron: "0 0 * * *"

and then you can have a dedicated job just for creating the "installation" matrix

    strategy:
      fail-fast: false
      matrix: ${{ fromJSON(needs.prepare-install.outputs.install-matrix) }}

@rebornplusplus
Copy link
Copy Markdown
Author

That is an interesting idea. I tried to do that. But the problem is that matrix will need to be different based on whether the workflow has been called via workflow_call or not. Maybe it's just me, but I didn't find any way to distinguish that, from within the reusable workflow. Let me know if you have some suggestions.

I added another input to remove duplication. Please let me know what you think!

hum there's quite a lot of repetition here. You don't need the two separate workflows.

One is enough, where:

on:
  workflow_call:
    inputs:
      install-all:
        description: "If true, install all slices in slices/"
        type: boolean
        default: false
        required: false
  push:
    branches:
      - "main"
    paths:
      - ".github/**"
  pull-request:
    branches:
      - "main"
    paths:
      - ".github/**"
  schedule:
    # Run at 00:00 every day.
    # Ref: https://man7.org/linux/man-pages/man5/crontab.5.html
    - cron: "0 0 * * *"

and then you can have a dedicated job just for creating the "installation" matrix

    strategy:
      fail-fast: false
      matrix: ${{ fromJSON(needs.prepare-install.outputs.install-matrix) }}

Copy link
Copy Markdown
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the matrix need to be different?

in both cases the matrix is:

    strategy:
      fail-fast: false
      matrix:
        branch: []
        arch: []

So you'll just need a job, before the matrix one, to create that map and pass it, as a JSON output, to the matrix job

@rebornplusplus
Copy link
Copy Markdown
Author

Why does the matrix need to be different?

in both cases the matrix is:

    strategy:
      fail-fast: false
      matrix:
        branch: []
        arch: []

If I have a PR (or push) targeted at ubuntu-22.04 branch, there needs not be any branch in the matrix there. It should simply run against that PR ref (or branch).

@cjdcordeiro
Copy link
Copy Markdown
Collaborator

But that's precisely where you can apply DRY. When there's a PR or a push against a specific branch, your matrix should look like:

    strategy:
      fail-fast: false
      matrix:
        branch: ["ubuntu-22.04"]
        arch: [amd64, ...]

And then your checkout action simply needs to use that ref

@rebornplusplus
Copy link
Copy Markdown
Author

On push, that matrix will work, yeah. But I don't think it will for PRs though. Because, if you "checkout" the ubuntu-22.04 branch for a PR targeted at that branch, you won't be able to determine the changed files in that PR. Because the files will not be checked out. The new files will be part of the refs/pull/<pr_number>/merge github.ref.

@cjdcordeiro
Copy link
Copy Markdown
Collaborator

cjdcordeiro commented Feb 14, 2024

On push, that matrix will work, yeah. But I don't think it will for PRs though. Because, if you "checkout" the ubuntu-22.04 branch for a PR targeted at that branch, you won't be able to determine the changed files in that PR. Because the files will not be checked out. The new files will be part of the refs/pull/<pr_number>/merge github.ref.

So you're saying that what you have now won't work?
https://github.com/canonical/chisel-releases/pull/119/files#diff-78bf4a33beef5f0e82b2661553d1e9fa2c2a17a5508f3be8eb5f1bf2e2fb1766R38-R41

Cause you are already checkout out the ref given as an input. If that works, what's the problem with putting that variable in a matrix?

@rebornplusplus
Copy link
Copy Markdown
Author

Added the changes as discussed in our sync: 8682974. Please take a look.

@rebornplusplus
Copy link
Copy Markdown
Author

Do you have any test runs?

Yup.

  1. Installing all slices: https://github.com/rebornplusplus/chisel-releases/actions/runs/7911557498/job/21595963180
    • This is triggered by push to the main branch.
  2. Installing slices changed in a PR: https://github.com/rebornplusplus/chisel-releases/actions/runs/7868931941/job/21596112188?pr=2

@cjdcordeiro
Copy link
Copy Markdown
Collaborator

Do you have any test runs?

Yup.

  1. Installing all slices: https://github.com/rebornplusplus/chisel-releases/actions/runs/7911557498/job/21595963180

    • This is triggered by push to the main branch.
  2. Installing slices changed in a PR: https://github.com/rebornplusplus/chisel-releases/actions/runs/7868931941/job/21596112188?pr=2

great, thanks.

That made me realize something though -> the test should be fixed such that it ignores the arch if there's no deb for it upstream. Otherwise we'll get this everyday: https://github.com/rebornplusplus/chisel-releases/actions/runs/7911557498/job/21595963413, which is a false positive

@rebornplusplus
Copy link
Copy Markdown
Author

That made me realize something though -> the test should be fixed such that it ignores the arch if there's no deb for it upstream. Otherwise we'll get this everyday: https://github.com/rebornplusplus/chisel-releases/actions/runs/7911557498/job/21595963413, which is a false positive

Are you practically saying to ignore these errors on a chisel cut run?

error: slice package "dotnet-host" missing from archive

(I wonder if there is any other way to check if the deb exists or not for a particular Ubuntu release, components and suites other than checking the chisel cut error.)

If so, then I can add an option to the script (something like --ignore-pkg-missing) which we should only use when we are trying to install all slices. If we use the option for PRs, then we might have contributors adding packages like foo which does not exist anywhere.

@cjdcordeiro
Copy link
Copy Markdown
Collaborator

check if the deb exists or not for a particular Ubuntu release

y this is what I'm saying. If it doesn't exist, it shouldn't throw an error. Like, .net only exists for arm64 and amd64, so, we'll be seeing errors everyday because of that, when in reality it's not an error. The script should only test the chisel cut for slices whose packages exist upstream, for the given release and arch

This commit re-writes the "install-slices" script from bash to python,
for better maintainability and easier addition of new features and
requirements.
@rebornplusplus
Copy link
Copy Markdown
Author

I have added the option to ignore missing packages.

  • When installing all slices, if a package has not been found in the archive, there will be no errors.
  • In other cases, where we are only installing changed slices/paths, missing packages will not be ignored and will result in an error like before. This is useful in PRs

I have also re-written the "install-slices" script from bash to python for better maintainability.


Example run for a PR: https://github.com/rebornplusplus/chisel-releases/actions/runs/7868931941/job/21646126428?pr=2
Example run for installing all slices: https://github.com/rebornplusplus/chisel-releases/actions/runs/7928229576/job/21646097589

@rebornplusplus rebornplusplus requested review from cjdcordeiro and removed request for cjdcordeiro February 16, 2024 09:10
Copy link
Copy Markdown
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the option to ignore missing packages.

  • When installing all slices, if a package has not been found in the archive, there will be no errors.
  • In other cases, where we are only installing changed slices/paths, missing packages will not be ignored and will result in an error like before. This is useful in PRs

I have also re-written the "install-slices" script from bash to python for better maintainability.

Example run for a PR: https://github.com/rebornplusplus/chisel-releases/actions/runs/7868931941/job/21646126428?pr=2 Example run for installing all slices: https://github.com/rebornplusplus/chisel-releases/actions/runs/7928229576/job/21646097589

I'm wondering...

  • When installing all slices, if a package has not been found in the archive, there will be no errors.

I tend to agree, but what if the package is removed from upstream? Like, completely...which afaik can happen for dev releases (noble atm). Then we would end up with a SDF in the release that doesn't work, at all.

I think the check should be: the package must exist for at least 1 arch. If it doesn't exist, at all, for a given release, then we'll be shooting ourselves in the foot everytime we open a new release, cause we won't be able to easily port the previous release onto the new one...not without potentially adding SDFs for packages that don't exist.

  • In other cases, where we are only installing changed slices/paths, missing packages will not be ignored and will result in an error like before. This is useful in PRs

Why? It's the same rationale as above...someone could try to add an SDF for a package that doesn't belong on that release.


I think there should be a check, before the multi-arch matrix, to validate that the slices can be tested - i.e. the package exists in the release.

@rebornplusplus
Copy link
Copy Markdown
Author

rebornplusplus commented Feb 19, 2024

I have changed the script. The current behaviour of the script is the following:

  • if --ignore-missing is set, it will first check if all the packages specified in the files passed are available for any arch in the archive. If it is, then it will filter out the arch-specific missing packages and only install whichever packages are available for that arch.
  • otherwise, if unset, there will be no such checks. It will dumbly try to install every slice in the files.

We are only passing the --ignore-missing flag whenever we are installing all slices. Because of the new change, we will be assured that no package has been removed from the archive. And for scenarios where we are not passing --ignore-missing, we can be assured that whatever slice definition files we pass to the script, the script will try to install.
In short, we will know if we have any estranged packages in our repo/PRs.


Usage info of the script
usage: install-slices [-h] --arch ARCH --release RELEASE [--ignore-missing] [file ...]

Verify slice definition files by installing the slices

positional arguments:
  file               Chisel slice definition file(s)

options:
  -h, --help         show this help message and exit
  --arch ARCH        Package architecture
  --release RELEASE  Chisel-releases branch name or directory
  --ignore-missing   Ignore arch-specific package not found in archive errors. If set, also
                     ensure that packages exist for any arch.

Example runs:

Copy link
Copy Markdown
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a few additional changes you should also be able to significantly increase the pylint score.

Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Comment thread .github/scripts/install-slices Outdated
Rafid Bin Mostofa added 3 commits February 22, 2024 12:29
This commit improves the install_slices script. The commit also
re-organises the files -- all the relevant files are put under
".github/scripts/install_slices" directory now. It also adds a new file
"pkglist" which contains the required debian packages for running the
script smoothly.
@rebornplusplus
Copy link
Copy Markdown
Author

Comment thread .github/scripts/install_slices/pkglist
Copy link
Copy Markdown
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! thanks! lgtm. let's see it in action

@cjdcordeiro
Copy link
Copy Markdown
Collaborator

I'll merge it once we get a 👍 from @linostar

Copy link
Copy Markdown

@linostar linostar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work!

@cjdcordeiro cjdcordeiro merged commit a7aeba3 into canonical:main Feb 22, 2024
gregory-schiano pushed a commit to gregory-schiano/chisel-releases that referenced this pull request Jun 24, 2024
NWarila pushed a commit to nwarila-platform/chisel-releases that referenced this pull request Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority Look at me first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants