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

feat: add an option to choose between staged-dependencies and setup-r-dependencies #252

Merged

Conversation

walkowif
Copy link
Contributor

@walkowif walkowif commented Sep 9, 2024

Example of workflows adapted to select setup-r-dependencies as the dependency installation method:

@walkowif walkowif marked this pull request as ready for review September 10, 2024 11:58
@walkowif walkowif requested a review from a team as a code owner September 10, 2024 11:58
@walkowif walkowif requested a review from pawelru September 10, 2024 11:58
Copy link
Contributor

github-actions bot commented Sep 10, 2024

Unit Tests Summary

3 tests   3 ✅  6s ⏱️
3 suites  0 💤
1 files    0 ❌

Results for commit bbaf1eb.

♻️ This comment has been updated with latest results.

@pawelru
Copy link
Contributor

pawelru commented Sep 10, 2024

This all is awesome right now. I have one more comment that would help to speed up the things - is to use PPM instead of default CRAN. This would allow us to install from binaries as opposed to download sourced - build binary - install. The difference will be significant.
Unfortunately, this is not a parameter of the setup-r-dependency action (it is in the setup-r which is meant to be run before but we are not using it currently). There should be many ways of doing this (maybe env var? or .Rprofile?). I would ask you to research this and suggest the best option.


UPDATE:
We can also add this to our setup-r-dependencies action as well. Up to you where you consider the most suitable place

@walkowif walkowif marked this pull request as draft September 12, 2024 09:30
@walkowif
Copy link
Contributor Author

I suggested a solution for using the binary packages from PPM in insightsengineering/setup-r-dependencies#6.

@walkowif walkowif marked this pull request as ready for review September 12, 2024 11:37
@walkowif walkowif marked this pull request as draft September 24, 2024 16:43
@walkowif
Copy link
Contributor Author

walkowif commented Sep 25, 2024

This PR is still used for testing insightsengineering/setup-r-dependencies#8. Once it's merged and new setup-r-dependencies action version is released

uses: insightsengineering/setup-r-dependencies@setup-binary-package-repository-for-different-platform

can be replaced with

uses: insightsengineering/setup-r-dependencies@v1

and this PR can be merged.

Edit: this is done.

…ry-for-different-platforms"

This reverts commit 67f8871.
@walkowif walkowif marked this pull request as ready for review September 26, 2024 08:15
Copy link
Contributor

@pawelru pawelru left a comment

Choose a reason for hiding this comment

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

This looks good from my end 👍

@cicdguy cicdguy merged commit b7c75cf into main Sep 27, 2024
26 checks passed
@cicdguy cicdguy deleted the choose-between-staged-dependencies-and-setup-r-dependencies branch September 27, 2024 10:32
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants