-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
2ec1631
to
3aa36d8
Compare
Test s3 auth and dry-run upload look good: I'll drop the tip commit. |
257e4e7
to
c25c264
Compare
Drive by comment: We have an open issue about dropping those two packages that can be closed once this is merged: |
Thanks for the reminder. Updated that commit to auto-close that issue. |
2290138
to
44ad032
Compare
Pushed update to pull shared actions from new repo at https://github.com/metrumresearchgroup/actions. |
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 |
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.
I've rebased and resolved the conflicts. Notable changes:
|
I said:
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. |
@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 |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments
Closes #550.
There was a problem hiding this 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
@barrettk Thanks for the careful review. |
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.