-
Notifications
You must be signed in to change notification settings - Fork 290
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
Remove pre
from the main action and move it to azure/login/clear-credentials
#474
Comments
Fixes misspellings identified by the [check-spelling action](https://github.com/marketplace/actions/check-spelling). The misspellings have been reported at https://github.com/jsoref/playwright/actions/runs/10015023629#summary-27685777352 The action will report that the changes in this PR would make it happy: https://github.com/jsoref/playwright/actions/runs/10015023971#summary-27685778305 --- I understand that the commit messages will need to be reworded to match house style. For the time being, these are merely noting the changes they contain so that when I rebase or need to drop things, I can. -- I've already rebased once as someone fixed one of the items that my draft work was going to fix. --- ## Testing * The tests _mostly_ passed when I managed to trigger them, but there were a handful of things that I didn't quite understand * There are a large number of warnings relating to a bad interaction between any workflow that uses this local action https://github.com/microsoft/playwright/blob/b535139b3292885dccf9e27ba2753c6c8d337c16/.github/actions/run-test/action.yml#L74-L80 and the action it calls -- I've opened Azure/login#474 asking them to refactor their action so that it doesn't cause so much noise while running this repository's tests * I'm vaguely curious as to why this repository has a `branch` constraint for its `pull_request` events in its workflows -- that constraint gave me a number of additional headaches while trying to prepare this branch for this PR. --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
I second this ; our use case is we're using
However, the |
Hi @jsoref, @olivierlefloch and @CK-KrzysztofKot, we've released a new version v2.2.0. In this version, we removed the |
Thanks @YanaXu, I believe this addresses my concerns. |
#431 is considering reworking things a bit.
I propose kicking
pre
out of the main action:action.yml
... runs: using: 'node20' - pre: 'lib/cleanup/index.js' main: 'lib/main/index.js' post: 'lib/cleanup/index.js'
and create
clear-credentials/action.yml
:Note: I'm not sure that
login.svg
is doing anything here. https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#brandingicon lists the valid values andlogin.svg
isn't on the list (nor is there alogin.svg
in the repository). (There doesn't appear to be documentation on this.)This would be a breaking change, so you'd bump the major version of
azure/login
...This would mean that workflows such as https://github.com/microsoft/playwright/blob/b8546eb35ea23c735be656b5675988cbc5486c7b/.github/workflows/tests_primary.yml that use local actions as noted in actions/runner#3397 would stop triggering dozens of warnings they can't do anything about.
But it would still allow people concerned about credentials that resulted in #384 to be able to clear credentials at start and have more than one login step.
The text was updated successfully, but these errors were encountered: