Skip to content

Added support for AzureGermanCloud#3030

Closed
horrion wants to merge 3 commits intoAzure:AutoRestfrom
horrion:AutoRest
Closed

Added support for AzureGermanCloud#3030
horrion wants to merge 3 commits intoAzure:AutoRestfrom
horrion:AutoRest

Conversation

@horrion
Copy link
Copy Markdown
Contributor

@horrion horrion commented Apr 5, 2017

Description


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as a link to the swagger spec, used to generate the code.
  • The project.json and AssemblyInfo.cs files have been updated with the new version of the SDK.

@azuresdkci
Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@msftclas
Copy link
Copy Markdown

msftclas commented Apr 5, 2017

@horrion,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link
Copy Markdown

msftclas commented Apr 5, 2017

@horrion, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@hovsepm
Copy link
Copy Markdown
Contributor

hovsepm commented Apr 5, 2017

@azuresdkci test this please

@hovsepm
Copy link
Copy Markdown
Contributor

hovsepm commented Apr 5, 2017

@horrion Hi Robert. Thank you for the contribution! We would like to better understand your usage scenarios because Authentication library in this repo is not supported anymore. We moved all the functionality to azure-powershell repo because it was used primarily there.

@markcowl
Copy link
Copy Markdown
Member

markcowl commented Apr 5, 2017

@horrion Note that, if you wanted to make a contribution that impacts the current libraries, an update here: https://github.com/Azure/azure-sdk-for-net/blob/AutoRest/src/ClientRuntime/Microsoft.Rest.ClientRuntime.Azure.Authentication/ActiveDirectoryServiceSettings.cs would be accepted

@shahabhijeet
Copy link
Copy Markdown
Contributor

@azuresdkci add to whitelist

1 similar comment
@hovsepm
Copy link
Copy Markdown
Contributor

hovsepm commented Apr 10, 2017

@azuresdkci add to whitelist

@horrion
Copy link
Copy Markdown
Contributor Author

horrion commented Apr 11, 2017

{AzureEnvironment.Endpoint.GraphEndpointResourceId, AzureEnvironmentConstants.USGovernmentGraphEndpoint}
}
}
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@horrion We have no plans to update this library, so please revert this.

private static readonly ActiveDirectoryServiceSettings AzureSettings = new ActiveDirectoryServiceSettings
{
AuthenticationEndpoint= new Uri("https://login.windows.net/"),
AuthenticationEndpoint= new Uri("https://login.microsoftonline.com/"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

login.windows.net is preferred.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@markcowl Could you please explain why the "old" endpoint is preferred? I thought everything that depends on Azure Resource Manager has to go through the new endpoint (login.microsoftonline.com).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@horrion You are absolutely right, I was reading this entirely the wrong way

Copy link
Copy Markdown
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, I love to see this kinf of proactive support. We are deprecating the AutoRest branch, but once this is ready, either you or we can move this into the new branch.

Also, need to update the library version so that we cna publish a new nuget package with the changes.

Thanks again for the contribution!

@markcowl
Copy link
Copy Markdown
Member

@horrion Note that the ci failure is due to adding the new environment in common.authentication without updating one of the tests - since this part of the change should be reverted, the tests should pass once this is done.

@horrion
Copy link
Copy Markdown
Contributor Author

horrion commented Apr 21, 2017

@markcowl I'll take care of removing the new environment in common.authentication.
Would you mind notifying me when the new branch becomes available?

@azuresdkci
Copy link
Copy Markdown
Contributor

The AutoRest branch is retired and no longer takes pull requests. vs17Dev is the new autorest based .net sdk branch located here: Azure/azure-sdk-for-net. Please submit pull requests to that branch.

@azuresdkci
Copy link
Copy Markdown
Contributor

The AutoRest branch is retired and no longer takes pull requests. vs17Dev is the new autorest based .net sdk branch located here: Azure/azure-sdk-for-net. Please submit pull requests to that branch.

@horrion
Copy link
Copy Markdown
Contributor Author

horrion commented May 8, 2017

@markcowl Thanks for your support, so far! I have just created a new PR for the vs17Dev branch here: #3182

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.

6 participants