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.

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

ci: switch to GitHub Actions #681

Merged
merged 9 commits into from
Jun 27, 2024
Merged

ci: switch to GitHub Actions #681

merged 9 commits into from
Jun 27, 2024

Conversation

kyleam
Copy link
Collaborator

@kyleam kyleam commented Apr 29, 2024

This series switches the CI from Drone to GitHub Actions. The goal is to preserve important features, including coverage measurement and the "oldest" job.

The main switch happens in the third commit. The fourth commits takes care of gh-550 by adding a check_pkgdown() step.


  • Drop tip commit that tests dry-run s3 sync.

  • After approval and right before merge, adjust the required status checks in the branch protection settings.

  • Coordinate with open pull requests. Once this is merged, these PRs should bring this in (by merge or rebase) to move away from Drone.

@kyleam kyleam force-pushed the gh-actions branch 3 times, most recently from 2ec1631 to 3aa36d8 Compare April 29, 2024 23:32
@kyleam
Copy link
Collaborator Author

kyleam commented Apr 29, 2024

Test s3 auth and dry-run upload look good:

I'll drop the tip commit.

@kyleam kyleam marked this pull request as ready for review April 29, 2024 23:40
@kyleam kyleam requested a review from seth127 April 29, 2024 23:42
.github/workflows/main.yaml Outdated Show resolved Hide resolved
@kyleam kyleam force-pushed the gh-actions branch 3 times, most recently from 257e4e7 to c25c264 Compare May 3, 2024 17:07
@barrettk
Copy link
Collaborator

barrettk commented May 3, 2024

Drive by comment: We have an open issue about dropping those two packages that can be closed once this is merged:
#629

@kyleam
Copy link
Collaborator Author

kyleam commented May 3, 2024

We have an open issue about dropping those two packages that can be closed once this is merged:
#629

Thanks for the reminder. Updated that commit to auto-close that issue.

@kyleam kyleam force-pushed the gh-actions branch 2 times, most recently from 2290138 to 44ad032 Compare May 8, 2024 16:32
@kyleam
Copy link
Collaborator Author

kyleam commented May 8, 2024

Pushed update to pull shared actions from new repo at https://github.com/metrumresearchgroup/actions.

@kyleam kyleam requested review from barrettk and removed request for seth127 May 9, 2024 21:14
@kyleam
Copy link
Collaborator Author

kyleam commented Jun 13, 2024

I'll return this to a draft because 1) the plan is to hold off on this until after the upcoming release and 2) I'll need to resolve the conflict and make a tweak to the oldest build for the addition of mrgmisc dependency.

For reference, here's a scratch actions run that brings in the current tip of feat/vpc (f81cf2d): https://github.com/metrumresearchgroup/bbr/actions/runs/9504620205

@kyleam kyleam marked this pull request as draft June 13, 2024 18:29
For the GitHub Actions switch, we're moving to
r-lib/actions/setup-r@v2, which uses pak underneath.  The pak
dependency resolution fails due to mrgvalidate and mrgvalprep.

The reason for the failure isn't clear to me, but given that this
project no longer uses these packages for validation, just remove them
as dependencies.  Also drop the stale yaml files related to the old
validation setup.

Closes #629.
Many CI platforms, including Drone and GitHub Actions, set the CI
environment variable.  Use that instead of DRONE so that these helpers
are a bit less tied to the particular CI platform.
7ddc09e (print the model type under the status for each model,
2024-06-04), part of the 1.11.0 release, introduced a call to
cli::col_br_magenta(), which isn't available until cli 3.1.0.
b70f320 (add remainder of tests & minor refactors/bug fixes caught
while writing them, 2024-05-06), part of the 1.11.0 release,
introduced a call to withr::local_seed(), which isn't available until
withr 2.4.0.
The 1.11.0 release started usign purrr::list_c() and
purrr::list_rbind(), which aren't available until purrr v1.0.0.
Since bf5c8c5 (use_bbi: add kludge for fs < 1.4.2, 2023-02-23), we've
had a compatibility kludge to support path_real() for fs releases
before 1.4.2 (released June 2020).  Any of the version constraints
added by the previous three commits require a newer snapshot than the
one needed to get fs 1.4.2, so bump the minimum fs version and drop
the kludge.
The goal is for the new setup to largely follow the Drone builds, but
there are some notable differences:

 * The Drone pipelines used custom images built to track Metworx's
   Ubuntu version and bake in many dependencies.

   The GitHub Actions use GitHub-hosted runners and test with the two
   most recent Ubuntu LTS releases.  r-lib/actions is used to install
   R and dependencies at the time of the run (with the built-in
   caching of r-lib/actions/setup-r-dependencies) and to execute 'R
   CMD check'.

   On the oldest build, a date-pinned RSPM URL is used, matching what
   was configured for the Drone image.

 * The mpn-latest builds have been dropped.  It'd be possible to mimic
   that behavior by pointing the setup-r CRAN value to MPN and setting
   a date-pinned RSPM.  However, the mpn-latest builds arguably don't
   provide much value, and any unlikely mpn-latest-specific
   incompatibilities will be caught before release by our standard
   release process.  So let's avoid the extra complexity unless it
   becomes obvious the builds are useful.

 * 'R CMD check' is ran for the R versions matching the two most
   recent R versions on Metworx _and_ on the latest R version.

 * For a PR, the Drone setup had two builds, one triggered by the
   branch push (testing on branch tip) and another by the PR
   (testing on merge with base).

   For the new setup, there is just one build triggered by the PR.
   It's done on the merge of the PR branch and its base.

   This means that the tip of the PR branch isn't tested.  In general,
   that's fine because it's the merge is what ultimately will land.
   And for the cases where testing on the tip of the branch is useful
   (e.g., debugging a discrepancy due to a semantic merge conflict), a
   'scratch/*' branch pointing to the same spot can be pushed.
@kyleam
Copy link
Collaborator Author

kyleam commented Jun 27, 2024

I've rebased and resolved the conflicts.

Notable changes:

  • The 4.0.5 and 4.1.3 builds have been updated to 4.2.3 and 4.3.1 to match the two most recent R versions on the latest AMI. R 4.0.5 is still tested in the "oldest" job.

  • The oldest build now uses the 2023-01-25 snapshot. An increase is required due to functionality that bbr v1.11.0 started using, and this particular snapshot is required to get purrr v1.0.0.

    Relevant commits:

    • e670827 (DESCRIPTION: specify minimum version for cli)
    • 34c469e (DESCRIPTION: specify minimum version for withr)
    • 4df2765 (DESCRIPTION: specify minimum version for purrr)
    • 5673079 (ci: increase 'oldest' build to MPN 2023-01-25)

@kyleam kyleam marked this pull request as ready for review June 27, 2024 15:08
@kyleam
Copy link
Collaborator Author

kyleam commented Jun 27, 2024

I said:

The oldest build now uses the 2023-01-25 snapshot

I think everyone is agreement that we are long overdue for a bump here. There may be some disagreement about whether 2023-01-25 is too recent. However, at this point, this is the effective minimum snapshot since the 1.11.0 release.

So my vote would be for discussion of that to not hold up merging this PR. We can create a dedicated issue to discuss whether we want to make changes to push back the minimum required snapshot.

@barrettk
Copy link
Collaborator

@kyleam I went through all your recent commits/refactors and agree with your perspective on opening a dedicated PR if we wanted to dial back the oldest snapshot/revisit version requirements. A shame we missed some of these things by deleting the mpn-oldest checks, but im also glad we're finally bumping the required cli version (purrr bump is nice too). Fingers crossed those two dont get dialed back, though I agree the oldest snapshot does feel a bit recent (I personally have no objections given how MPN works though).

.github/workflows/main.yaml Outdated Show resolved Hide resolved
The previous commit used the 2020-06-08 MPN snapshot for the "oldest"
check job to match what Drone used for its "oldest" pipeline before
the pipeline was removed by 352db78 (remove CRAN-oldest from drone
entirely, 2024-05-02).

However, following that commit, several spots started using
functionality not present in the 2020-06-08 snapshot, leading to bumps
in the minimum versions of cli (requires 2021-11-19 snapshot), purrr
(requires 2023-01-25), and withr (requires 2021-02-01).

Given the above, the minimum required snapshot is now 2023-01-25.
Copy link
Collaborator

@barrettk barrettk left a comment

Choose a reason for hiding this comment

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

Left some comments

.github/workflows/main.yaml Outdated Show resolved Hide resolved
.github/workflows/main.yaml Show resolved Hide resolved
.github/workflows/main.yaml Show resolved Hide resolved
.github/workflows/main.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@barrettk barrettk 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 for the changes and replies. Left one more point of clarity

.github/workflows/main.yaml Show resolved Hide resolved
@kyleam
Copy link
Collaborator Author

kyleam commented Jun 27, 2024

@barrettk Thanks for the careful review.

@kyleam kyleam merged commit 4e9822a into main Jun 27, 2024
8 checks passed
@kyleam kyleam deleted the gh-actions branch June 27, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants