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.

Sign up for GitHub

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

Jump to bottom

SDK changes to support new version of swagger Resources API 2020-06-01 #13081

Merged
merged 10 commits into from
Jul 10, 2020
Merged

SDK changes to support new version of swagger Resources API 2020-06-01 #13081

merged 10 commits into from
Jul 10, 2020

Conversation

Xynoclafe
Copy link
Contributor

@Xynoclafe Xynoclafe commented Jun 29, 2020 •
edited
Loading

SDK changes to support new version of swagger Resources API 2020-06-01

There's some work being done on the service side which has to go in before this is merged, but I'd like to start the review off on this.

@Xynoclafe Xynoclafe changed the title Xynoclafe/resources sdk2020 06 SDK changes to support new version of swagger Resources API 2020-06-01 Jun 29, 2020
@isra-fel isra-fel added Do Not Merge Mgmt This issue is related to a management-plane library. labels Jun 30, 2020
@isra-fel
Copy link
Member

LGTM. Waiting for service status to be updated.

filizt
filizt reviewed Jun 30, 2020
View reviewed changes
sdk/resources/Microsoft.Azure.Management.Resource/src/Generated/ITagsOperationOperations.cs Outdated
/// <summary>
/// TagsOperationOperations operations.
/// </summary>
public partial interface ITagsOperationOperations
Copy link
Contributor

Choose a reason for hiding this comment

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

ublic partial interface ITagsOperationOperations [](start = 5, length = 48)

After the changes we reverted on Tags, I'd think the name of the interface would be "ITagsOperations" - could you check on this please?

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 think this is leftover from the time we generated the SDK before reverting those changes. I don't think it's being used anywhere (I see that ITagsOperations exists separately), I should be able to just remove it. I will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just looked into it and yes, they were leftover from previous SDK generation as the TagsOperations files did not replace them. The class are not referenced anywhere outside their own definitions/extensions and build/tests pass successfully even on removing them.

filizt
filizt reviewed Jun 30, 2020
View reviewed changes
.../Microsoft.Azure.Management.Resource/tests/ScenarioTests/TemplateSpecsTests.ScenarioTests.cs Outdated
namespace ResourceGroups.Tests

{
class LiveTemplateSpecsTest : TestBase
Copy link
Contributor

Choose a reason for hiding this comment

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

class LiveTemplateSpecsTest : TestBase [](start = 4, length = 38)

We should probably communicate with @stuartko and add Template Spec tests.

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 had created this just as a placeholder of sorts. I've already talked to him about tests.

@filizt
Copy link
Contributor

filizt commented Jun 30, 2020 

public class LiveDeploymentTests : TestBase

Do you mind adding a test for the statusMessage update we made?

Refers to: sdk/resources/Microsoft.Azure.Management.Resource/tests/ScenarioTests/DeploymentTests.ScenarioTests.cs:18 in fd9e77b. [](commit_id = fd9e77b, deletion_comment = True)

@Xynoclafe
Copy link
Contributor Author 

public class LiveDeploymentTests : TestBase

Do you mind adding a test for the statusMessage update we made?

Refers to: sdk/resources/Microsoft.Azure.Management.Resource/tests/ScenarioTests/DeploymentTests.ScenarioTests.cs:18 in fd9e77b. [](commit_id = fd9e77b, deletion_comment = True)

I added a test for this in InMemoryTests. I was waiting for statusMessage update to be fixed completely before adding a test in LiveDeploymentTests.

@Xynoclafe Xynoclafe requested a review from AlexGhiondea as a code owner July 1, 2020 22:02
@filizt
Copy link
Contributor

filizt commented Jul 2, 2020

LGTM

@isra-fel
Copy link
Member

isra-fel commented Jul 7, 2020

Anything else to add to this PR?
PS there are merging conflicts

@Xynoclafe
Copy link
Contributor Author

Anything else to add to this PR?
PS there are merging conflicts

I think we have everything we need. We should be okay to merge this now. I'll push a commit fixing merge issues in a while.

@Xynoclafe
Copy link
Contributor Author

All changes are in.

@shenglol
Copy link
Contributor

shenglol commented Jul 8, 2020

@isra-fel mind helping merge the PR? We have another minor SDK update for resources that is pending on this.

@isra-fel isra-fel merged commit 2aa2698 into Azure:master Jul 10, 2020
@Xynoclafe Xynoclafe mentioned this pull request Jul 10, 2020
Merged
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-net that referenced this pull request Feb 26, 2021
@sumitmal2711 @sumitmal
@mayankagg9722 @vityagi @deymadhumanti @madhude
CodeGen from PR 13168 in Azure/azure-rest-api-specs
93250b6 
Users/mayaggar/dataprotection (Azure#13168)

* New Readme Config File

* New Go Language Readme Config File

* New Azure AZ Readme Config File

* New Azure CLI Readme Config File

* New Typescript Language Readme Config File

* New Python Language Readme Config File

* New C# Language Readme Config File

* New AzureResourceSchema Readme Config File

* New Swagger Spec File

* New Swagger Example Spec File

* Microsoft.data protection (Azure#12814)

* moving changes from Private repo

* changes for autorest vqalidation err

* fix delete response

* fixing lint and model errors

* exposureControlledFeatures fixes

* prettier fixes

* fixing spell check issues

* adding backuptype in custom-words

* PolicyParameters related changes for Disk Backup

* fixing PR comments

* description changes

* changes for preview to stable folder

* changes for retention in monitoring

* changes for stable in readme

* fixing checklist gate issues

* changes for systemdata in dppresource

Co-authored-by: sumitmal <32121310+sumitmal@users.noreply.github.com>
Co-authored-by: Mayank Aggarwal <mayaggar@microsoft.com>
Co-authored-by: vityagi <vityagi@microsoft.com>

* MFA MUA DPP swagger changes (Azure#13081)

* MFA MUA DPP swagger changes

* Resolving PR comments

Co-authored-by: Madhumanti Dey <madhude@microsoft.com>

* Swagger changes for VaultGuardResource Object (Azure#13116)

* MFA MUA DPP swagger changes

* Resolving PR comments

* MFA MUA dpp swagger changes

* Fixed preetierCheck failures

* Fixed Avocado failures

* Fixed LintDiff warning

* resolved PR comments

* GO SDK fix

* Go SDK fix

* Go SDK fix

* preview related changes

* Go SDK fix

* resolved PR comments

Co-authored-by: Madhumanti Dey <madhude@microsoft.com>

* changes for preview DPP version

* fix for credscan SAS

Co-authored-by: sumitmal2711 <58544969+sumitmal2711@users.noreply.github.com>
Co-authored-by: sumitmal <32121310+sumitmal@users.noreply.github.com>
Co-authored-by: Mayank Aggarwal <mayaggar@microsoft.com>
Co-authored-by: vityagi <vityagi@microsoft.com>
Co-authored-by: deymadhumanti <58032062+deymadhumanti@users.noreply.github.com>
Co-authored-by: Madhumanti Dey <madhude@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filizt filizt filizt left review comments

@isra-fel isra-fel isra-fel approved these changes

@erich-wang erich-wang Awaiting requested review from erich-wang

@AlexGhiondea AlexGhiondea Awaiting requested review from AlexGhiondea

Assignees
No one assigned
Labels
Do Not Merge Mgmt This issue is related to a management-plane library.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Xynoclafe @isra-fel @filizt @shenglol @stuartko