Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CI: migrate integration-test to GH actions #1969

Merged
merged 3 commits into from
Jan 26, 2023
Merged

CI: migrate integration-test to GH actions #1969

merged 3 commits into from
Jan 26, 2023

Conversation

aciba90
Copy link
Contributor

@aciba90 aciba90 commented Jan 18, 2023 •
edited
Loading

Proposed Commit Message

ci: migrate integration-test to gh actions

changes:
- drop caching machinery for LXD images and chroots
- divide the workflow in two steps, one for building packages
  and other for running the tests so that the second one can
  be retried in case of error

Additional Context

I have not ported the caching machinery for LXD images and chroots, as in my opinion it is not worth the complexity vs the gains, but I am open to other opinions.

@aciba90 aciba90 force-pushed the gh-integration-tests branch 15 times, most recently from 20f233f to 603a568 Compare January 23, 2023 19:34
@aciba90 aciba90 force-pushed the gh-integration-tests branch from 603a568 to 3bed2c2 Compare January 24, 2023 09:22
@aciba90 aciba90 changed the title WIP: CI: migrate integration-test to GH actions CI: migrate integration-test to GH actions Jan 24, 2023
@aciba90 aciba90 marked this pull request as ready for review January 24, 2023 10:05
@aciba90 aciba90 requested a review from paride January 24, 2023 10:13
@paride
Copy link
Contributor

paride commented Jan 24, 2023

Nice, I agree with not implementing caching for now, that's something that can be added later. I'll have a better look later today. @aciba90 did you manage to test this on a fork or something?

@aciba90
Copy link
Contributor Author

aciba90 commented Jan 24, 2023

@aciba90 did you manage to test this on a fork or something?

Thanks, @paride. The new workflow is being executed as part of the checks of this PR. See the Checks tab or https://github.com/canonical/cloud-init/actions/runs/3994716010/jobs/6852827704.

paride
paride approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@paride paride left a comment

Choose a reason for hiding this comment

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

Looks great! Just one minor inline question.

.github/workflows/integration.yml Outdated
- name: "Checkout #2 (for tools/read-version)"
run: |
git fetch --unshallow
git remote add upstream https://git.launchpad.net/cloud-init
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to add LP as a remote, especially given that it is added but never fetched?

Copy link
Contributor Author

@aciba90 aciba90 Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That was probably a relic / copy-paste, I have removed it and unified Checkout #1 and #2 in a single one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice and TIL about fetch-depth, thanks!

@aciba90 aciba90 force-pushed the gh-integration-tests branch from b5be300 to 984efee Compare January 25, 2023 09:33
paride
paride reviewed Jan 25, 2023
View reviewed changes
Copy link
Contributor

@paride paride left a comment

Choose a reason for hiding this comment

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

One more inline comment/question.

.github/workflows/integration.yml Outdated
runs-on: ubuntu-22.04
steps:
- name: "Checkout"
uses: actions/checkout@v3.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Here (and below, also for other actions), is there a reason for not going with @v3 instead of v3.x.y, relying on the fact that compatibility shouldn't be broken in a given major version?

The official docs (e.g. https://github.com/actions/checkout) only specify the major.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason was to keep it consistent with the rest of workflows, but taking into account that they are official GitHub actions, I agree it is better to pin only major versions. Updated, thanks for pointing out that.

I will align the other workflows to this approach in a follow-up PR.

paride
paride approved these changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@paride paride left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

TheRealFalcon
TheRealFalcon approved these changes Jan 25, 2023
View reviewed changes
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Looks good! Just curious, is this split into 2 separate jobs with the upload and download so that we can download the built deb when investigating failures? Or is there different reason for the split?

@aciba90
Copy link
Contributor Author

aciba90 commented Jan 26, 2023

Looks good! Just curious, is this split into 2 separate jobs with the upload and download so that we can download the built deb when investigating failures? Or is there different reason for the split?

Thanks, @TheRealFalcon. That's one of the reason plus:

  • If the integration-test step fails, we can retry it without executing the package-build step.
  • Separation of concerns. It is more clear what dependencies are required for each step.

@TheRealFalcon TheRealFalcon merged commit 28b8703 into canonical:main Jan 26, 2023
@aciba90 aciba90 deleted the gh-integration-tests branch January 26, 2023 13:47
@TheRealFalcon TheRealFalcon mentioned this pull request Sep 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paride paride paride approved these changes

@TheRealFalcon TheRealFalcon TheRealFalcon approved these changes

@holmanb holmanb Awaiting requested review from holmanb

@blackboxsw blackboxsw Awaiting requested review from blackboxsw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aciba90 @paride @TheRealFalcon