Skip to content

Conversation

@mingqishao
Copy link
Contributor

@mingqishao mingqishao commented May 12, 2023

Description of your changes

Today the Azure provider are using a static scope in token. In future the different scope need to be used for the flexible scenario.

This changes are:

  1. add a new --scope arguments in refresh-token. The new scope argument will be pass to Azure token provider.
  2. For the backward compatibility, the scope argument is optional. The old static scope will be used by default.

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

I added unit test for changes codes

Special notes for your reviewer

@mingqishao mingqishao changed the title add "--scope" argument for refresh-token add scope argument for refresh-token May 12, 2023
@mingqishao mingqishao changed the title add scope argument for refresh-token a new "scope" argument for refresh-token May 12, 2023
@mingqishao mingqishao changed the title a new "scope" argument for refresh-token feat: a new "scope" argument for refresh-token May 12, 2023
@mingqishao mingqishao requested a review from Ealianis May 12, 2023 15:30
Comment on lines 26 to 29
const (
aksAADApplicationID = "6dae42f8-4368-4678-94ff-3960e28e3630"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

why move the azure specific const to the main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved back the const to pkg/authtoken/providers/azure/azure_msi.go, and keep the default scope value logic there too.

Copy link
Contributor

@ryanzhang-oss ryanzhang-oss left a comment

Choose a reason for hiding this comment

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

The new scope field is not even assigned.

…ify the type

2. move the default scope logic to pkg/authtoken/providers/azure/azure_msi.go
@mingqishao
Copy link
Contributor Author

The new scope field is not even assigned.

I found that too. That is shame. I have fixed it.

@mingqishao mingqishao changed the title feat: a new "scope" argument for refresh-token feat: a new "scope" argument on for refresh-token May 15, 2023
@mingqishao mingqishao changed the title feat: a new "scope" argument on for refresh-token feat: a new "scope" argument for refresh-token May 15, 2023
@ryanzhang-oss ryanzhang-oss merged commit 05c132f into main May 15, 2023
@zhiying-lin zhiying-lin deleted the minsha/add-scope-argument branch August 16, 2023 08:41
britaniar pushed a commit to britaniar/fleet that referenced this pull request Jan 20, 2026
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