-
Notifications
You must be signed in to change notification settings - Fork 4.1k
[ContainerRegistry] Fix -ExposeToken only print token as secure string without login in Connect-AzContainerRegistry
#29007
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
base: main
Are you sure you want to change the base?
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
e440d3e to
ed07a71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the -ExposeToken parameter behavior in the Connect-AzContainerRegistry cmdlet. Previously, when using -ExposeToken, the cmdlet would still perform a docker login and call the Authenticate() method. The fix changes the behavior to only return the access token as a SecureString without performing the login, allowing users to manually authenticate with containerization tools like docker.
Key changes:
- Modified the ExposeToken logic to return the password (refresh token) as a SecureString instead of calling
Authenticate() - Removed the
statusfield from the returned credential object when ExposeToken is used - Added Example 4 to the help documentation demonstrating how to use the ExposeToken feature with docker login
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ContainerRegistry/ContainerRegistry/Commands/ConnectAzureContainerRegistry.cs | Updated ExposeToken logic to convert password to SecureString and removed the status field from the returned object |
| src/ContainerRegistry/ContainerRegistry/help/Connect-AzContainerRegistry.md | Added Example 4 showing how to use ExposeToken with docker login |
| src/ContainerRegistry/ContainerRegistry/ChangeLog.md | Added changelog entry documenting the ExposeToken fix |
| - Additional information about change #1 | ||
| --> | ||
| ## Upcoming Release | ||
| * Fixed `-ExposeToken` with `Connect-AzContainerRegistry` |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ChangeLog entry needs to be more descriptive and user-facing. It should explain what the issue was and how it affects users, not just state that something was fixed. Additionally, it should reference the related GitHub issue if one exists. For example: "Fixed -ExposeToken parameter in Connect-AzContainerRegistry to properly return the access token without performing docker login" or similar.
| var cred = new | ||
| { | ||
| status = this.ExecuteScript<object>(LoginScript), | ||
| loginServer = Name + ".azurecr.io", | ||
| accessToken = this.RegistryDataPlaneClient.Authenticate() | ||
| accessToken = this.Password.ConvertToSecureString() | ||
| }; |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of the status field from the returned credential object is a breaking change. The previous implementation included a status field that contained the result of the docker login script execution. Removing this field without documentation could break existing scripts that depend on this return value. Consider whether this field should be retained for backward compatibility, or if this is an intentional breaking change, it should be clearly documented in the ChangeLog with the breaking change format.
ed07a71 to
2ca8c8d
Compare
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.