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

Remove pre from the main action and move it to azure/login/clear-credentials #474

Closed
jsoref opened this issue Jul 19, 2024 · 3 comments
Closed
Assignees
Labels
product enhancement New feature or request

Comments

@jsoref
Copy link
Contributor

jsoref commented Jul 19, 2024

#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:

name: 'Azure Login: Clear Credentials'
description: 'Clear Azure credentials (use this once before your first `uses: azure/login` step)'
branding:
  icon: 'login.svg'
  color: 'blue'
runs:
   main: 'lib/cleanup/index.js' 

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 and login.svg isn't on the list (nor is there a login.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.

@jsoref jsoref added the need-to-triage Requires investigation label Jul 19, 2024
@MoChilia MoChilia self-assigned this Jul 22, 2024
@MoChilia MoChilia added product enhancement New feature or request and removed need-to-triage Requires investigation labels Jul 22, 2024
dgozman pushed a commit to microsoft/playwright that referenced this issue Jul 22, 2024
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>
@olivierlefloch
Copy link

I second this ; our use case is we're using azure/login in an internal re-usable, configurable setup workflow which does not always login with Azure. So the azure/login invocation is conditionally enabled:

    - name: "Azure login"
      if: inputs.auth-with-azure == 'true'
      uses: azure/login@v2
      …

However, the pre step is run in all cases, which incurs overhead for all our jobs.

@YanaXu YanaXu assigned YanaXu and unassigned MoChilia Aug 15, 2024
@YanaXu
Copy link
Collaborator

YanaXu commented Sep 18, 2024

Hi @jsoref, @olivierlefloch and @CK-KrzysztofKot, we've released a new version v2.2.0. In this version, we removed the pre step and added 2 env variables to enable/disable pre or post cleanup.
v2 is aligned with v2.2.0 now.
Please try the new version. You can also refer to Enable/Disable the cleanup steps.

@jsoref
Copy link
Contributor Author

jsoref commented Sep 18, 2024

Thanks @YanaXu, I believe this addresses my concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants