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(r): Refactor CI configuration #1210

Merged
merged 27 commits into from
Oct 20, 2023
Merged

Conversation

paleolimbot
Copy link
Member

@paleolimbot paleolimbot commented Oct 18, 2023

This PR moves R workflows into their own file since they don't depend on any other jobs. It also adds an r-extended job that checks some of the more obscure things (e.g., older R versions, valgrind). The gist of the CI setup is:

  • r-basic.yml: Makes sure that drivers build + pass tests with test databases on Linux. These run when C, R, or Go code is modified. This catches changes to the drivers.
  • r-standard.yml: Checks driver manager and driver packages on Mac, Windows, and Linux. These run when working on the R packages because that is the time that MacOS and Windows-specific regressions are likely to occur (e.g., when working on the build systems).
  • r-extended.yml: Checks the way CRAN would check (on oldrel, release, and devel versions of R on Mac, Windows, and Linux); checks older R versions still in the tidyverse support grid on Windows (because the build system changes so frequently) and Linux (because Go drivers don't build on older R versions on Windows); runs tests with valgrind.

Closes #1138.

I opened tickets to debug the valgrind errors reported by the drivers. Those CI jobs run weekly (reflecting the time I have available to debug them) so they won't add noise to ongoing development.

@paleolimbot paleolimbot marked this pull request as ready for review October 19, 2023 12:45
fail-fast: false

env:
GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

Any secrets should not be set globally as envvars (neither workflow nor job wide) and only set in the step they are used. That way potentially compromised actions can't grab them. Not essential here as the permissions are set to read only.

Copy link
Member Author

Choose a reason for hiding this comment

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

That must have been a hangover from some earlier workflow (I think maybe pak needs it for setup-r-dependencies but we don't install anything from GitHub at the moment)

Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

image
😮

Looks very good permissions and everything is there 👍 .

Theoretically you could move the sequence of checkout, setup r, setup deps, check package into a local action/reusable workflow which would reduce the non valgrind jobs in r-extended to one call to that and reduce duplication. But there is also something to say for listing the steps clearly without hiding behind an abstraction... so 🤷

@lidavidm
Copy link
Member

105 checks might be a bit excessive...they are all short, but 50% of the runtime of each of them is just setting up R at this point

@lidavidm
Copy link
Member

Oh, right, but many of them are weekly and not on every PR. That's better then.

@paleolimbot
Copy link
Member Author

Ok, I think I got it down to 5 checks that run on every PR, which should catch most things from changes outside the r/ directory that would break the packages. It's true that setting up R has higher overhead than setting up Python...I'm happy to re-workshop these if that becomes a problem (but since most of them just run weekly I don't think it will come up).

@paleolimbot paleolimbot merged commit abf67e9 into apache:main Oct 20, 2023
108 of 109 checks passed
@paleolimbot paleolimbot deleted the r-ci-workflow branch October 20, 2023 14:35
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.

chore(r): Add test run with valgrind
3 participants