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

Consider removing notice annotations for authentication scheme #192

Closed
bgilbert opened this issue Nov 18, 2023 · 6 comments · Fixed by #196
Closed

Consider removing notice annotations for authentication scheme #192

bgilbert opened this issue Nov 18, 2023 · 6 comments · Fixed by #196

Comments

@bgilbert
Copy link

twine-upload.sh logs some notice annotations reporting the mechanism it uses to authenticate to the repository. These appear on the summary page for the GitHub Actions workflow:

image

This information is useful for debugging purposes, but please consider reporting it via a regular log message instead of a notice annotation, so it doesn't appear on the workflow summary page. Annotations typically describe unusual conditions that the user should be aware of, such as non-fatal errors or pending deprecations. In this case, twine-upload.sh is just reporting that it authenticated normally, using the mechanism it was configured to use, so the annotation could mislead a user into thinking that a problem has occurred.

@woodruffw
Copy link
Member

I'm of two minds on this: I agree that the annotation here is pretty noisy, but we also settled on it because Trusted Publishing is pretty implicit: it's pretty easy to use a configured API token by accident. Having a notice is a quick visual confirmation that the workflow is succeeding because of TP, rather than because it happens to still be using an old API token.

OTOH, Trusted Publishing has now been deployed for over 6 months and people probably understand it better than they did before. So the annotation probably isn't doing much anymore 🙂

@webknjaz
Copy link
Member

Information level annotations are indeed for normal operation. Don't confuse with warnings which are meant to hint that something's not right/deprecated.

The publish job is running once in a while so I don't think it's that noisy, honestly. Also, in cases when each merge to main results in a release to something like TestPyPI, it may be a confirmation that it did happen.

Anyway, the mode information is important to have without forcing restarts of such jobs. So it must at least be present in the log output.

Note that the workflow summary page may get much more info from other sources, like jobs summary. Here's some examples:

@bgilbert
Copy link
Author

@webknjaz Note that both of those workflow runs only have warning annotations, except for the notice from this Action. Markdown summaries are more freeform and I'd expect those to include arbitrary informational reports.

You're right that notices and warnings have different semantics, but most Actions don't post an annotation to confirm that they ran normally. We don't see annotations like:

  • ℹ️ Set up Python 3.11.6
  • ℹ️ Retrieved 312.1 MiB from cache key "dependency-cache"
  • ℹ️ 317 tests passed, 12 skipped

even though each of those messages contains potentially useful information. That information is available from the logs if needed. In this case, I'd argue that because a release job is important, it's more likely that a human will cross-check the summary page, so there's a higher cost to emitting confusing messages there.


If you do keep the message, there are a couple possible improvements:

  • Simplify the text. Something like Authenticating to PyPI via Trusted Publishing is clearly normal and non-actionable, while Attempting to [...] retrieve a temporary short-lived API token [...] due to __token__ username with no supplied password field suggests a potential misconfiguration or fallback path.
  • Add a quiet boolean input to disable the message for users who want that.

I should also say: thank you for this Action! Setting up automatic publishing for my Python package was completely painless, and it's awesome (and unexpected!) that I didn't need to configure an API key to do it.

@webknjaz
Copy link
Member

webknjaz commented Dec 3, 2023

Thanks for the kind words! A lot of people worked together to ensure the secretless publishing is smooth :)

Regarding the annotation, maybe we should use ::debug instead https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-debug-message. But I'd still want to have this information in the console log.

Not sure about rephrasing. @woodruffw WDYT?

As a side note, I'd like to make use of job summaries eventually. So if you have any ideas in that context, feel free to share.

@woodruffw
Copy link
Member

Regarding the annotation, maybe we should use ::debug instead https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-debug-message. But I'd still want to have this information in the console log.

This makes sense to me!

Not sure about rephrasing. @woodruffw WDYT?

I like the rephrasing -- I agree that the current phrasing could be read as ambiguously actionable, which is more confusing than strictly necessary 🙂

As a side note, I'd like to make use of job summaries eventually. So if you have any ideas in that context, feel free to share.

One thing that comes to mind is that it would be cool/nice to have a job summary listing the following for each distribution uploaded:

  1. Distribution name
  2. Distribution URL on PyPI
  3. Distribution digests (SHA256, BLAKE2b maybe?)

@webknjaz
Copy link
Member

One thing that comes to mind is that it would be cool/nice to have a job summary listing the following for each distribution uploaded:

  1. Distribution name

As in a filename? Should the twine output be piped there? (and wrapped with <details>, and <code>)

  1. Distribution URL on PyPI

There's a related FR @ #97

  1. Distribution digests (SHA256, BLAKE2b maybe?)

There's a toggle for printing these out to the console, so it'd need to be reused, I guess: https://github.com/pypa/gh-action-pypi-publish#showing-hash-values-of-files-to-be-uploaded.

(alternatively, this could become enabled by default with the setting deprecated)

webknjaz added a commit that referenced this issue Dec 20, 2023
This replaces the use of `::notice` in each authentication case with `::debug`, reducing the user confusion caused by the these messages. It also simplifies the message in the Trusted Publishing case to make it less ambiguous.

Closes #192.
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 a pull request may close this issue.

3 participants