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: upgrade artifact actions to v4 #1399

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

Conversation

gatoniel
Copy link

fixes #1398

Comment on lines +102 to +104
{% raw %}
name: coverage-data-${{ matrix.python }}-${{ matrix.os }}
{% endraw %}
Copy link

Choose a reason for hiding this comment

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

Why are these % raw tags needed?

Copy link
Author

Choose a reason for hiding this comment

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

Otherwise the line would look like this:

          name: coverage-data-${{"{{"}} matrix.python {{"}}"}}-${{"{{"}} matrix.os {{"}}"}}

This was suggested by @webknjaz

Choose a reason for hiding this comment

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

@bosd it's usually cleaner to wrap chunks of mustache that shouldn't be rendered this way. I find that seeing {{ '{{' }} all over the place doesn't help readability.

@bosd
Copy link

bosd commented Nov 16, 2024

I think a codecov token should be added as well.
As we did over here: py-pdf/pypdf_table_extraction@f494056

@gatoniel
Copy link
Author

I think a codecov token should be added as well. As we did over here: py-pdf/pypdf_table_extraction@f494056

Shouldn't this be done in a separate pull request as it is about bumping codecov/[email protected] to version 4.

@webknjaz
Copy link

@gatoniel tokens are mandatory in v4 and v5 of the action (by the way, v5 was released several days ago so maybe you could bump to that).

Though, secrets are inaccesible in forks and this upload method tends to make uploads in PRs from forks less stable. The solution many have found is storing the token in plain text instead of repo secrets. It's not that secret, anyway. I like putting it into the config file codecov.yml so it's not copied across several action invocations.

OTOH, Codecov has recently introduced an org-level toggle on the web UI to disable requiring tokens for uploads. I haven't tried it but it seems like it might eliminate the need to use tokens again..

@gatoniel
Copy link
Author

I can try to also add the codecov version bump to this PR. But I thought, this could be done in a different PR as this PR is dedicated to bumping the artifact upload/download action.

@webknjaz
Copy link

Oh, I got confused and thought you were bumping codecov for a minute. I that case, it's out of the scope, you're right.

@gatoniel
Copy link
Author

gatoniel commented Nov 17, 2024

Oh, I got confused and thought you were bumping codecov for a minute. I that case, it's out of the scope, you're right.

I don't mind doing it, in a separate PR.

But is this project going to be continued or are there other cookiecutters getting more attention for python projects as this one is not supporting major new developments in the python community? I am too outside of the active community to see these developments :( But, I've heard people stop using poetry and switching to other builders, version managers?

@webknjaz
Copy link

There's one by Simon Willison: https://github.com/simonw/python-lib

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.

Deprecation notice: v3 of the artifact actions
3 participants