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

Add pre: and post: action for cleaning up #384

Merged
merged 12 commits into from
Dec 28, 2023
Merged

Conversation

MoChilia
Copy link
Member

@MoChilia MoChilia commented Dec 14, 2023

Description

This pr will add pre: and post: action for cleaning up to solve the issue #383 and #374.

The pre: and post: action run at the start and the end of a job respectively, instead of running cleaning up azure accounts at the beginning of the main: action. This enables users to switch active azure accounts logged in by azure/login within the job, see scenario in #383. Meanwhile, it ensures a more clean and secure environment for login, see #376 (comment).

Release tests:
https://github.com/Azure/azclitools-actions-test/actions/runs/7345677123
https://github.com/Azure/azclitools-actions-test/actions/runs/7345708979

Example:
image

Since setting environment variable for agent is required in current process, there is no need to use core.exportVariable. See https://github.com/actions/toolkit/blob/main/packages/core/README.md#exporting-variables, core.exportVariable is used to add environment variable to this step and future steps environment blocks.

@MoChilia MoChilia changed the title Add pre: and post: action for clearing accounts Add pre: and post: action for cleaning up Dec 14, 2023
@MoChilia MoChilia self-assigned this Dec 26, 2023
@MoChilia MoChilia requested a review from YanaXu December 26, 2023 09:35
@MoChilia MoChilia marked this pull request as ready for review December 26, 2023 09:35
@MoChilia MoChilia merged commit b503882 into master Dec 28, 2023
25 checks passed
@MoChilia MoChilia deleted the sy/pre-post-cleanup branch December 28, 2023 09:00
MoChilia added a commit that referenced this pull request Jan 8, 2024
MoChilia added a commit that referenced this pull request Jan 8, 2024
@will-beta-clique
Copy link

Got the following errors from a self-deployed github action runner:

Error: Login cleanup failed with Error: Unable to locate executable file: az. Please verify either the file path exists or the file can be found within a directory specified by the PATH environment variable. Also check the file mode to verify the file is executable.. Make sure 'az' is installed on the runner. If 'enable-AzPSSession' is true, make sure 'pwsh' is installed on the runner together with Azure PowerShell module.

Wonder how to prepare az tool in the workflow files to fix this error.

@MoChilia
Copy link
Member Author

Hi @will-beta-clique, have you installed azure-cli on your self-deployed runner? You can refer to https://learn.microsoft.com/en-us/cli/azure/install-azure-cli.

@szberes
Copy link

szberes commented Jan 16, 2024

Got the following errors from a self-deployed github action runner:

Error: Login cleanup failed with Error: Unable to locate executable file: az. Please verify either the file path exists or the file can be found within a directory specified by the PATH environment variable. Also check the file mode to verify the file is executable.. Make sure 'az' is installed on the runner. If 'enable-AzPSSession' is true, make sure 'pwsh' is installed on the runner together with Azure PowerShell module.

Wonder how to prepare az tool in the workflow files to fix this error.

We are seeing the same issue. Previously we installed the azure cli as a separate step in the workflow before the login.

@opdude
Copy link

opdude commented Jan 16, 2024

As this happens in a PRE step this is causing any workflows running on runners without az pre-installed to fail even if we have a step for installing az. I would revert this PR and make a new release frankly.

@benjamin-rousseau-shift

Yup nice for breaking all workflow running self hosted runners. The pre step don't even let you any chance to install az cli :/

@kristeey
Copy link

Bug issue: #403

@MoChilia
Copy link
Member Author

Hi @will-beta-clique @szberes @opdude @benjamin-rousseau-shift @kristeey, please first pin the version to v1.5.1. We will prepare a fix by skipping cleanup if az is not installed. And Let's track the issue in #403.

@lawrence-witt
Copy link

Having another issue with this release this morning. Can no longer use dynamically retrieved output values for credentials using the github actions JSON tooling, since the function is executed before the pipeline can populate it:

name: Login to Azure
uses: azure/login@v1
with:
  creds: |
    {
      "clientId":"${{ secrets.gh_clientid }}",
      "clientSecret":"${{ secrets.gh_clientsecret }}",
      "subscriptionId":"${{ fromJSON(steps.subscriptions.outputs.result).targetId }}",
      "tenantId":"${{ secrets.tenantid }}"
    }
  enable-AzPSSession: true

Results in Error reading JToken from JsonReader. Path '', line 0, position 0. during the pre step.

@pasokan-intel
Copy link

i see the same issue when we use secrets to be fetched from previous step of the workflow 'fromJSON(steps.xyz)'
(Line: 246, Col: 18): Error reading JToken from JsonReader. Path '', line 0, position 0.

@Siddeshgad
Copy link

Bug issue: Issue 405

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.

10 participants