Skip to content

Conversation

@MoChilia
Copy link
Member

@MoChilia MoChilia commented Jan 15, 2025

Setting AZURE_CONFIG_DIR on runners may disrupt the Azure/CLI action when Azure/login is pre-executed for sign in. In this pr, to align with the logic in Azure CLI, we will mount AZURE_CONFIG_DIR folder instead of ~/.azure, and reset the user's Azure configuration directory to /root/.azure within the container.

Background: Due to a breaking change made in the latest GitHub runner image, AZURE_CONFIG_DIR was set to /opt/az-config. Since Azure/cli used to mount the ~/.azure folder as the user's Azure configuration directory, this will cause Azure/cli to not work with Azure/login.

Test: https://github.com/Azure/azclitools-actions-test/actions/runs/12804625062/job/35699389204

@MoChilia MoChilia requested review from YanaXu and jiasli January 15, 2025 05:26
@MoChilia MoChilia self-assigned this Jan 15, 2025
src/main.ts Outdated
scriptFileName = await createScriptFile(inlineScript);

const azureConfigDir = process.env.AZURE_CONFIG_DIR || path.join(process.env.HOME, '.azure');
process.env.AZURE_CONFIG_DIR = '/root/.azure';
Copy link
Member

Choose a reason for hiding this comment

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

This changes the AZURE_CONFIG_DIR env var of the host.

If any Azure CLI command is run after docker run is executed, it will see AZURE_CONFIG_DIR as /root/.azure which doesn't exist on the host.

I would recommend exactly setting AZURE_CONFIG_DIR env var of the docker process via -e, --env, instead of inheriting from process.env.

Copy link
Member Author

Choose a reason for hiding this comment

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

No Azure CLI command will run after executing docker run. According to the Node.js doc, process.env changes do not affect the environment outside the Node.js process or other worker threads. Setting process.env.AZURE_CONFIG_DIR = '/root/.azure'; only impacts the current process and is not passed to subsequent processes.

Using the -e flag to set AZURE_CONFIG_DIR would complicate the code, as it requires omitting AZURE_CONFIG_DIR on the host and then explicitly mounting it. I don't think this approach is more efficient than resetting AZURE_CONFIG_DIR within the current process.

Copy link
Member

Choose a reason for hiding this comment

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

The most efficient way is to not passing AZURE_CONFIG_DIR to the container at all.

@MoChilia
Copy link
Member Author

Although GitHub runner image team has reverted their change, I still believe it is necessary to align with the logic in Azure CLI.

src/main.ts Outdated
- Set the working directory for docker continer
- volume mount the GITHUB_WORKSPACE env variable (path where users checkout code is present) to work directory of container
- voulme mount .azure session token file between host and container,
- voulme mount Azure config directory between host and container,
Copy link
Member

@jiasli jiasli Jan 16, 2025

Choose a reason for hiding this comment

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

Suggested change
- voulme mount Azure config directory between host and container,
- volume mount Azure config directory between host and container,

src/main.ts Outdated
args.push("-e", `${key}=${process.env[key]}`);
}
}
args.push("-e", `AZURE_CONFIG_DIR=${containerAzureConfigDir}`);
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? I think not inheriting AZURE_CONFIG_DIR into the container is enough.

@MoChilia MoChilia changed the title Fix #175: Mount AZURE_CONFIG_DIR folder instead of ~/.azure Mount AZURE_CONFIG_DIR folder instead of ~/.azure Jan 16, 2025
@MoChilia MoChilia merged commit 48a3fb0 into Azure:master Jan 16, 2025
3 checks passed
@MoChilia MoChilia deleted the azure_config_dir branch January 16, 2025 08:32
oskogstad pushed a commit to Altinn/dialogporten that referenced this pull request Oct 1, 2025
> [!NOTE]
> Mend has cancelled [the proposed
renaming](https://redirect.github.com/renovatebot/renovate/discussions/37842)
of the Renovate GitHub app being renamed to `mend[bot]`.
> 
> This notice will be removed on 2025-10-07.

<hr>

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [azure/CLI](https://redirect.github.com/azure/CLI) | action | minor |
`v2.1.0` -> `v2.2.0` |

---

### Release Notes

<details>
<summary>azure/CLI (azure/CLI)</summary>

###
[`v2.2.0`](https://redirect.github.com/Azure/cli/releases/tag/v2.2.0):
GitHub Action for Azure CLI v2.2.0

[Compare
Source](https://redirect.github.com/azure/CLI/compare/v2.1.0...v2.2.0)

##### What's Changed

- Mount `AZURE_CONFIG_DIR` folder instead of `~/.azure` by
[@&#8203;MoChilia](https://redirect.github.com/MoChilia) in
[Azure/cli#176](https://redirect.github.com/Azure/cli/pull/176)
- FIX: Broken links by appended dot azcliversion errors by
[@&#8203;nselpriv](https://redirect.github.com/nselpriv) in
[Azure/cli#178](https://redirect.github.com/Azure/cli/pull/178)

##### New Contributors

- [@&#8203;nselpriv](https://redirect.github.com/nselpriv) made their
first contribution in
[Azure/cli#178](https://redirect.github.com/Azure/cli/pull/178)

**Full Changelog**:
<Azure/cli@v2.1.0...v2.2.0>

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 7am on Sunday,before 7am on
Wednesday" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/Altinn/dialogporten).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xMzEuOSIsInVwZGF0ZWRJblZlciI6IjQxLjEzMS45IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

3 participants