Migrate to charmcraft 3 & `charmcraft test` #210

carlcsaposs-canonical · 2025-02-03T16:01:04Z

Ported from canonical/mysql-router-k8s-operator#379

taurus-forever

OMG. This is going to be more complex as initially planned.
LGTM, @carlcsaposs-canonical please share TODO list we need to address to simplify CI. E.g. I see tons of pendinds TODO for charmcraft, let's sync with Sergio&Co there.

It will be harded to simplify this once we scale it to all repos. LGTM.

taurus-forever · 2025-02-06T14:20:55Z

.github/workflows/integration_test.yaml

+        run: |
+          sudo snap install charmcraft --classic
+          pipx install tox poetry
+      - name: Collect spread jobs


Should it be a new DPW action ?

I don't think so since the contents of this workflow are quite coupled to the spread configuration & the creation of a reusable workflow would limit the ways in which spread can be used

canonical/mysql-router-k8s-operator#379 (comment)

taurus-forever · 2025-02-06T14:23:42Z

.github/workflows/release.yaml

    with:
      channel: dpe/edge
-      artifact-prefix: ${{ needs.build.outputs.artifact-prefix }}
+      artifact-prefix: ${{ needs.ci-tests.outputs.artifact-prefix }}


Are we now releasing artifacts built outside of our trusted machines.
This opens us to https://en.wikipedia.org/wiki/XZ_Utils_backdoor

huh? we're building on github runners, same as before

this change is to use same build for test + release (canonical/mysql-router-k8s-operator#366)

taurus-forever · 2025-02-06T14:32:36Z

charmcraft.yaml

+      # Workaround to add unique identifier (git hash) to charm version while specification
+      # DA053 - Charm versioning
+      # (https://docs.google.com/document/d/1Jv1jhWLl8ejK3iJn7Q3VbCIM9GIhp8926bgXpdtx-Sg/edit?pli=1)
+      # is pending review.


Let's complete this officially to move from here. Based on the last conversation I think we have synced all topics.

multi track on single git branch could affect this, but yes I think we're synced. still needs approval from Mykola/Mohamed, but think we should wait until we get back to refresh v3

this is the same as before this PR; no change in behavior in this pr

carlcsaposs-canonical · 2025-02-06T14:58:51Z

This is going to be more complex as initially planned

Worth noting that this PR includes the charmcraft 3 migration as well

TODO list we need to address to simplify CI. E.g. I see tons of pendinds TODO for charmcraft

don't see any easy paths forward here; happy to discuss

also the linked charmcraft issues only account for these extra lines (

mysql-router-operator/.github/workflows/integration_test.yaml

Lines 104 to 108 in ab1aa0a

    
                 # TODO: remove when https://github.com/canonical/charmcraft/issues/2105 and 
        
                 # https://github.com/canonical/charmcraft/issues/2130 fixed 
        
                 - run: | 
        
                     sudo snap install go --classic 
        
                     go install github.com/snapcore/spread/cmd/spread@latest

,

mysql-router-operator/.github/workflows/integration_test.yaml

Line 121 in ab1aa0a

run: ~/go/bin/spread -vv -artifacts=artifacts '${{ matrix.job.spread_job }}'

), no other complexity will be removed

paulomach · 2025-02-07T12:42:40Z

spread.yaml

+    type: adhoc
+    # Only run on CI
+    manual: true
+    # HACK: spread requires runners to be accessible via SSH


sinclert-canonical

I think some logic re-shuffling is needed, to ease the maintainability of the integration-test workflow going forward.

sinclert-canonical · 2025-02-07T13:40:58Z

.github/workflows/integration_test.yaml

+          import json
+          import os
+          import subprocess
+
+          spread_jobs = (
+              subprocess.run(
+                  ["charmcraft", "test", "--list", "github-ci"], capture_output=True, check=True, text=True
+              )
+              .stdout.strip()
+              .split("\n")
+          )
+          jobs = []
+          for job in spread_jobs:
+              # Example `job`: "github-ci:ubuntu-24.04:tests/spread/test_charm.py:juju36"
+              _, runner, task, variant = job.split(":")
+              # Example: "test_charm.py"
+              task = task.removeprefix("tests/spread/")
+              if runner.endswith("-arm"):
+                  architecture = "arm64"
+              else:
+                  architecture = "amd64"
+              # Example: "test_charm.py:juju36 | amd64"
+              name = f"{task}:{variant} | {architecture}"
+              # ":" character not valid in GitHub Actions artifact
+              name_in_artifact = f"{task}-{variant}-{architecture}"
+              jobs.append({
+                  "spread_job": job,
+                  "name": name,
+                  "name_in_artifact": name_in_artifact,
+                  "runner": runner,
+              })
+          output = f"jobs={json.dumps(jobs)}"
+          print(output)
+          with open(os.environ["GITHUB_OUTPUT"], "a") as file:
+              file.write(output)


I think moving this whole block to a Python script within .github/scripts would reduce the cognitive load of understanding what this job / set of steps does, in addition to having syntax highlighting.

AFAIK, the only thing that depends on GHA is the final population of a JSON string into the GITHUB_OUTPUT env var, which could be the output of such script.

would reduce the cognitive load of understanding

I think it would increase the cognitive load, since you need to look at 2 files at once to understand what's happening, especially with respect to the value of items in the matrix

I see the benefits for syntax highlighting/linting—if you want to open a separate PR for this, feel free; imo it shouldn't block this PR

Also, this isn't something I expect to be edited often

And there are some benefits to a single file for copy-paste across repos & avoiding issues with some files getting copy-pasted while others not, and sync issues

I think it would increase the cognitive load, since you need to look at 2 files at once to understand what's happening

Splitting code into multiple files is the principle in which complex logic can be understood and scoped. That is why there are multiple files within the src directory of this repo, instead of a single and massive charm.py...

there are some benefits to a single file for copy-paste across repos

I agree with this statement, but that does not necessarily make it good. Designing code so that it is easy to copy+paste may be a code smell, as if we need to copy+paste it multiple times, maybe it is a sign that we should make it distributable (i.e. A reusable GHA workflow + scripts).

I would have expressed these concerns earlier, probably in this PR, but I stayed away as you told me.

we should make it distributable (i.e. A reusable GHA workflow + scripts)

would you like to volunteer to create & maintain that distributable component?

sinclert-canonical · 2025-02-07T13:41:13Z

.github/workflows/integration_test.yaml

+          import dataclasses
+          import json
+          import pathlib
+
+
+          @dataclasses.dataclass(frozen=True)
+          class Result:
+              test_case_id: str
+              path: pathlib.Path
+
+              def __eq__(self, other):
+                  if not isinstance(other, type(self)):
+                      return False
+                  return self.test_case_id == other.test_case_id
+
+
+          actual_results = pathlib.Path("allure-results")
+          default_results = pathlib.Path("allure-default-results")
+
+          results: dict[pathlib.Path, set[Result]] = {
+              actual_results: set(),
+              default_results: set(),
+          }
+          for directory, results_ in results.items():
+              for path in directory.glob("*-result.json"):
+                  with path.open("r") as file:
+                      id_ = json.load(file)["testCaseId"]
+                  results_.add(Result(id_, path))
+
+          actual_results.mkdir(exist_ok=True)
+
+          missing_results = results[default_results] - results[actual_results]
+          for default_result in missing_results:
+              # Move to `actual_results` directory
+              default_result.path.rename(actual_results / default_result.path.name)


I think moving this whole block to a Python script within .github/scripts would reduce the cognitive load of understanding what this job / set of steps does, in addition to having syntax highlighting.

sinclert-canonical · 2025-02-07T13:44:52Z

.github/workflows/integration_test.yaml

+          # Reverse engineered from https://github.com/simple-elf/allure-report-action/blob/eca283b643d577c69b8e4f048dd6cd8eb8457cfd/entrypoint.sh
+          import json
+
+          DATA = {
+              "name": "GitHub Actions",
+              "type": "github",
+              "buildOrder": ${{ github.run_number }},  # TODO future improvement: use run ID
+              "buildName": "Run ${{ github.run_id }}",
+              "buildUrl": "https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}",
+              "reportUrl": "../${{ github.run_number }}/",
+          }
+          with open("allure-results/executor.json", "w") as file:
+              json.dump(DATA, file)


This block is relatively small, and highly dependent on GHA populated env variables. Here my suggestion would be to leverage existing GHA JSON utilities (see toJSON), instead of spawning a Python shell just to do a JSON dump.

I think python is slightly more forgiving than raw json syntax, and has equivalent readability

no arm charm built on 20

This reverts commit 99331db.

This reverts commit 0fea833.

This reverts commit 6a97140.

This reverts commit 8cfb3bd.

carlcsaposs-canonical force-pushed the charmcraft-3 branch from b2c9acc to 0fea833 Compare February 6, 2025 09:01

carlcsaposs-canonical requested review from paulomach, taurus-forever, shayancanonical and sinclert-canonical February 6, 2025 10:51

carlcsaposs-canonical marked this pull request as ready for review February 6, 2025 10:51

taurus-forever approved these changes Feb 6, 2025

View reviewed changes

paulomach reviewed Feb 7, 2025

View reviewed changes

spread.yaml

type: adhoc

# Only run on CI

manual: true

# HACK: spread requires runners to be accessible via SSH

Copy link

Contributor

paulomach Feb 7, 2025

Choose a reason for hiding this comment

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

oh dear

sinclert-canonical reviewed Feb 7, 2025

View reviewed changes

This was referenced Feb 7, 2025

Use charmcraft test & concierge canonical/mysql-operator#609

Open

Use charmcraft test & concierge canonical/mysql-k8s-operator#573

Open

carlcsaposs-canonical added 18 commits February 7, 2025 14:54

Use charmcraft 3

7d1a334

Use charmcraft test

d9cd5ca

Use charmcraft test

c9070bf

temp disable needs

1ced031

Fix unit tests

9c2fc87

Fix unit tests

fa18794

Fix variants

8c95e13

Fix allure

0b0e790

unit test timeout

709cd48

pass env

af0bdcf

remove juju 29 arm

955ff79

reduce unit test combinations

2ceb5e4

Use series instead of base

460a61c

pass env

56f8504

remove 36 arm ubuntu 20

c8ee644

no arm charm built on 20

fix test_architecture.py

7bd3ba8

inherit secrets

ef51da4

exclude focal

c89cce5

carlcsaposs-canonical added 16 commits February 7, 2025 14:54

fix test_architecture.py path

6be3f1e

fix env variables

dc29110

fix test_architecture.py on 20.04

831f87d

temp debug

444ffc2

Revert "temp debug"

642d5cc

This reverts commit 99331db.

fix test_architecture.py

bd604ff

latest/stable

cfee52f

Revert "latest/stable"

a6be031

This reverts commit 0fea833.

fix tls deploy

ac85a2b

Revert "fix tls deploy"

6b657ae

This reverts commit 6a97140.

fix tls deploy

52f9f50

fixup! fix tls deploy

d3375a7

Remove unused marker

8a9247a

Revert "temp disable needs"

b5a0171

This reverts commit 8cfb3bd.

Fix lxd cleanup

926db33

Fix upgrade test arm

b69e73d

carlcsaposs-canonical force-pushed the charmcraft-3 branch from b1657d4 to b69e73d Compare February 7, 2025 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to charmcraft 3 & `charmcraft test` #210

Migrate to charmcraft 3 & `charmcraft test` #210

carlcsaposs-canonical commented Feb 3, 2025 •

edited

Loading

taurus-forever left a comment

taurus-forever Feb 6, 2025

carlcsaposs-canonical Feb 6, 2025

taurus-forever Feb 6, 2025

carlcsaposs-canonical Feb 6, 2025

taurus-forever Feb 6, 2025

carlcsaposs-canonical Feb 6, 2025

carlcsaposs-canonical commented Feb 6, 2025

paulomach Feb 7, 2025

sinclert-canonical left a comment

sinclert-canonical Feb 7, 2025

carlcsaposs-canonical Feb 7, 2025

carlcsaposs-canonical Feb 7, 2025

sinclert-canonical Feb 7, 2025

carlcsaposs-canonical Feb 7, 2025

sinclert-canonical Feb 7, 2025

sinclert-canonical Feb 7, 2025

carlcsaposs-canonical Feb 7, 2025

Migrate to charmcraft 3 & charmcraft test #210

Are you sure you want to change the base?

Migrate to charmcraft 3 & charmcraft test #210

Conversation

carlcsaposs-canonical commented Feb 3, 2025 • edited Loading

taurus-forever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlcsaposs-canonical commented Feb 6, 2025

Choose a reason for hiding this comment

sinclert-canonical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Migrate to charmcraft 3 & `charmcraft test` #210

Migrate to charmcraft 3 & `charmcraft test` #210

carlcsaposs-canonical commented Feb 3, 2025 •

edited

Loading