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

Bump required R version to R 4.0.0 #584

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Bump required R version to R 4.0.0 #584

wants to merge 4 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Nov 7, 2024

(Since other tidyverse/r-lib packages will requiring it; this just makes the dependency explicit for covr)

(Since other tidyverse/r-lib packages will requiring it; this just makes the dependency explicit for covr)
@jimhester
Copy link
Member

I guess we should also stop testing on 3.6 in the CI along with this? https://github.com/r-lib/covr/actions/runs/11728979579/job/32673603649?pr=584

@hadley
Copy link
Member Author

hadley commented Nov 7, 2024

Oh yeah, let me fix that too.

- name: Setup
run: |
R CMD INSTALL .
source shim_package.sh
Copy link
Member

Choose a reason for hiding this comment

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

This is critically important to run covr on itself, because you get into an infinite recursion issue if you don't rename the package to something else before trying to run covr on it, which is what shim_package.sh does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I missed that.

shell: Rscript {0}

- uses: codecov/codecov-action@v4
Copy link
Member

Choose a reason for hiding this comment

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

What is the advantage of using the codecov-action vs having covr upload to codecov directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know 😞 I just updated using our standard scripts. @gaborcsardi might know.

Copy link
Member

Choose a reason for hiding this comment

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

covr uses the old protocol, which does not support organization level codecov tokens.

Copy link
Member

Choose a reason for hiding this comment

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

🤔

Copy link
Member

@jimhester jimhester Nov 8, 2024

Choose a reason for hiding this comment

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

Hmm, I guess due to this https://codecovpro.zendesk.com/hc/en-us/articles/21567817259163-Error-finding-token-when-trying-to-upload, but I have always just added the codecov app and never had any problems with the automatic token.

Copy link
Member

@gaborcsardi gaborcsardi Nov 8, 2024

Choose a reason for hiding this comment

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

Well, you don't have any problems if you don't look at the results. :) Upload failures do not fail the workflow run, so you typically do not notice them:

[1] "Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected time to availability: 1300s."

https://github.com/r-lib/covr/actions/runs/11729639825/job/32675733827#step:6:88

The last successful upload for covr was ~7 months ago:
https://app.codecov.io/gh/r-lib/covr/commits

Copy link
Member

Choose a reason for hiding this comment

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

I seem to no longer have access to the admin settings for this repo, so can't comment on if codecov app is still installed for this repo or the r-lib org in general.

But I don't really like going through the cobutura codepath for repositories by default, it adds some additional complexity and also a xml2 dependency. Additionally the output is really geared towards more java style classes, and doesn't perfectly align with R package organization. Additionally doesn't relying on secrets mean that forks are no longer supported?

I am not sure why the existing path would work differently with organization tokens vs the action? Aren't they both changing the secret into an environment variable in the same way?

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.

3 participants