Repository navigation
Conversation
lczyk
changed the title
zhijie-yang
left a comment
zhijie-yang
left a comment
There was a problem hiding this comment.
Looks good to me. Just a few nitpicks here.
cjdcordeiro
left a comment
cjdcordeiro
left a comment
There was a problem hiding this comment.
this doesn't feel like a sustainable solution.
I'd like to consider a smarter approach where we can detect the state of the release, and apply the ignore accordingly.
This way we avoid doomsdays where the CI just stops working cause a release went EOL, while also giving us room to introduce Pro CI runs for LTS releases that reach EOSS (like 20.04)
For now, just to get the CI passing, I think we can just:
|
| "arch": arch, | ||
| "ref": release["ref"], | ||
| "chisel-version": chisel_version, | ||
| "ignore-flag": determine_ignore_flag(release.get("ignore", ""), chisel_version) |
There was a problem hiding this comment.
Interestingly, we can use a one-liner for this function
| "ignore-flag": determine_ignore_flag(release.get("ignore", ""), chisel_version) | |
| "ignore-flag": "--ignore-{}".format(ig) if (ig := release.get("ignore") and chisel_version == "main") else "" |
There was a problem hiding this comment.
👎 don't like the := syntax. very unreadable and hard on debuggers
|
I have read the PR and I agree with the general approach. I agree with @cjdcordeiro that the most maintainable version would not hardcode data that depends on the dates. However, IMO that depends on the complexity of the solution. Changing the CI every year once does not feel that heavy.
This feels like it should be easy on the surface, parsing the dates from YAML and replicating the business logic even if there are some more complex cases (i.e. when pro is not enabled and this is an LTS look at With that said, both solutions look good to me. |
yeah, that was my initial idea, but dropping 'main' from 20.04 felt like a step back.
☝️ + we are already doing a manual matrix spec. i do agree that we want to be careful about not doing too many things by hand though. hard to tell what's the optimal thing to do here tbh. |
|
proposal: canonical/chisel#245 (4 line change) and then always pass both i suggest we go with exactly what @cjdcordeiro suggested for now, and then, if the above is accepted, just re-add "main" to 20.04 and always run with both flags |
cjdcordeiro
left a comment
cjdcordeiro
left a comment
There was a problem hiding this comment.
This PR is getting delays cause it's doing too much, for a fix that would be 4 lines:
diff --git a/.github/scripts/install-slices/install_slices.py b/.github/scripts/install-slices/install_slices.py
index c68b781..eb5e84c 100755
--- a/.github/scripts/install-slices/install_slices.py
+++ b/.github/scripts/install-slices/install_slices.py
@@ -285,7 +285,7 @@ def ignore_missing_packages(
def install_slices(
- chunk: list[tuple[str, str]], dry_run: bool, arch: str, release: str, worker: int
+ chunk: list[tuple[str, str]], dry_run: bool, arch: str, release: str, worker: int, extra_args: str
) -> None:
"""
Install the slice by running "chisel cut".
@@ -410,6 +410,7 @@ def main() -> None:
logging.info("No slices will be installed.")
return
+ extra_args = '--ignore=unstable' if subprocess.check_output(["chisel","version"]).strip().decode().lstrip("v") > "1.2.0" else ''
# TODO: this list of slices can still be reduced, since many of these
# will come as essentials of other slices. We could simply crawl all essentials
# in the release, and remove them from the "all_slices" list (since they
@@ -426,6 +427,7 @@ def main() -> None:
cli_args.arch,
cli_args.release,
i // chunk_size + 1, # worker number
+ extra_args
)
for i in range(0, len(all_slices), chunk_size)
]
diff --git a/.github/workflows/install-slices.yaml b/.github/workflows/install-slices.yaml
index 8d0b164..d9c997a 100644
--- a/.github/workflows/install-slices.yaml
+++ b/.github/workflows/install-slices.yaml
@@ -24,7 +24,7 @@ env:
ARCHES: '["amd64","arm64","armhf","ppc64el","riscv64","s390x"]'
RELEASES_COMPATIBILITY: |
[
- {"ref": "ubuntu-20.04", "chisel-versions": ["v1.1.0","v1.2.0","main"]},
+ {"ref": "ubuntu-20.04", "chisel-versions": ["v1.1.0","v1.2.0"]},
{"ref": "ubuntu-22.04", "chisel-versions": ["v1.1.0","v1.2.0","main"]},
{"ref": "ubuntu-24.04", "chisel-versions": ["v1.1.0","v1.2.0","main"]},
{"ref": "ubuntu-25.04", "chisel-versions": ["v1.1.0","v1.2.0","main"]},The rest of the smartness and refactoring should be done on a follow up PR, since this is a blocking issue that you want to get merged fast.
| # and the entry in RELEASES_COMPATIBILITY contains "main" in "chisel-versions" | ||
| jq_program='[.[] | select(.ref == $ref) | select(.["chisel-versions"] | index("main")) | ."chisel-versions" = ["main"]]' | ||
| ref="$(echo "$RELEASES_COMPATIBILITY" | jq --arg ref "$branch" "$jq_program")" |
There was a problem hiding this comment.
why? seems unnecessary
There was a problem hiding this comment.
otherwise it is trying to install with main on this PR (and will also on others like it). see here for example
how about #683? i would then rebase this PR on top |
|
closing this since necessary changes made in #683. can later port changes to another PR |
Proposed changes
adds
--ignore=unstableand--ignore=unmaintainedflag to theinstall-slicesci job.the unstable flag is required allow #681 to not fail (at the time of writing it is unstable)
the unmaintained flag allows #677 not to fail (at the time of writing it is past its maintenance period)
note: the CI checks are going to be red across the board due to the underlying issue, until we add the
maintenancefield.Related issues/PRs
#677 and #681 are part of a series of (independent) commits which implement #631, and resolves #675
Forward porting
n/a
Checklist