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.

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

Reservations RP 2018-06-01: Add new cosmosDb in the reservedResourceT… #4701

Closed
wants to merge 7 commits into from
Closed

Reservations RP 2018-06-01: Add new cosmosDb in the reservedResourceT… #4701

wants to merge 7 commits into from

Conversation

luluRagdoll
Copy link

@luluRagdoll luluRagdoll commented Aug 24, 2018

…ype enum. Add name property in Patch properties

Description

Add "CosmosDb" in the reservedResourceType enum
Add "name" property in the PatchProperties

Link to REST API spec PR: Azure/azure-rest-api-specs#3690

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 the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

…ype enum. Add name property in Patch properties
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Left a couple of comments

}
// BEGIN: Code Generation Metadata Section
public static readonly String AutoRestVersion = "latest";
public static readonly String AutoRestBootStrapperVersion = "(empty)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this manually modified?

Copy link
Author

@luluRagdoll luluRagdoll Aug 27, 2018

Choose a reason for hiding this comment

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

I think this is because I was using the generate.ps1
Start-AutoRestCodeGeneration -ResourceProvider "reservations/resource-manager" -AutoRestVersion "latest"
I used this to generate this PR. Do you think I need to change the autorestversion here from latest to "1.2.0-preview"

Copy link
Author

Choose a reason for hiding this comment

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

This is auto-generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Please feel free to let me know if there is anything else I need to do.

/// </summary>
[JsonProperty(PropertyName = "properties.name")]
public string Name { get; set; }

Copy link
Contributor

Choose a reason for hiding this comment

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

These look like additive changes.
Please update package version, assembly version and package realease notes in the csproj and AssemblyInfo.cs file

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Currently, it is 1.1.0-preview. Should it be updated to 1.2.0-preview?
AssemblyInfo.cs should be 1.0.2.0 ?

Copy link
Author

Choose a reason for hiding this comment

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

I also saw the AssemblyFileVersion currenlty as 1.8.0.0. Should I update that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

1.8.0-preview is the last published version.
Please update package version to 1.9.0-preview and AssemblyFileVersion in AssemblyInfo.cs to 1.9.0.0

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@dsgouda
Copy link
Contributor

dsgouda commented Aug 28, 2018

Please fix the CI build failures

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Please fix CI test failures

}
// BEGIN: Code Generation Metadata Section
public static readonly String AutoRestVersion = "latest";
public static readonly String AutoRestBootStrapperVersion = "(empty)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming

@dsgouda
Copy link
Contributor

dsgouda commented Aug 28, 2018

@luluRagdoll Like I pointed out please take a look at the CI failures and fix them

@luluRagdoll
Copy link
Author

luluRagdoll commented Aug 29, 2018 via email

@luluRagdoll
Copy link
Author

@dsgouda
Hi Deepak,
I checked the reason for the failure is the wrong position of '}'. in SdkInfo_AzureReservationAPI.cs.
// END: Code Generation Metadata Section }

However this is auto-generated by running generate.ps1:
Start-AutoRestCodeGeneration -ResourceProvider "reservations/resource-manager" -AutoRestVersion "latest"
Does that mean the autorest is having some issue auto-generating the sdkInfo?
Any suggestions on how I can fix that? Thx

@dsgouda
Copy link
Contributor

dsgouda commented Aug 30, 2018

@luluRagdoll Acknowledged the issue, will fix the code generator
Until then can you manually push that trailing } to the next line and update the PR
Apologize for the inconvenience

@luluRagdoll
Copy link
Author

@dsgouda Sure! Just updated that file manually. CI test is passing now.

@dsgouda
Copy link
Contributor

dsgouda commented Aug 30, 2018

@luluRagdoll everything looks great.
Please squash commits into a single commit and we are good to merge here

@luluRagdoll
Copy link
Author

@dsgouda
Looks like I cannot change the commits that are already pushed in the remote branch. So I squash the commits and created a new pull request: #4724

@luluRagdoll
Copy link
Author

Do you mean I can squash merge my change to Azure:psSdkJson6 myself?

@dsgouda
Copy link
Contributor

dsgouda commented Aug 30, 2018

Thanks, taking a look

@dsgouda dsgouda closed this Aug 30, 2018
@dsgouda dsgouda reopened this Aug 30, 2018
@dsgouda
Copy link
Contributor

dsgouda commented Aug 30, 2018

The new PR is way behind the psSdkJson6 branch. Please create a new branch with latest psSdkJson6 changes and copy over your changes to it. That PR should be good

@luluRagdoll
Copy link
Author

#4725 Just synced up with the latest version. This is the PR. Thx

@dsgouda dsgouda closed this Aug 30, 2018
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.

4 participants