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

Bug fix for CertificateInformation and add additional properties to ApiContract #4652

Merged
merged 3 commits into from
Aug 16, 2018
Merged

Bug fix for CertificateInformation and add additional properties to ApiContract #4652

merged 3 commits into from
Aug 16, 2018

Conversation

solankisamir
Copy link
Member

@solankisamir solankisamir commented Aug 10, 2018

Description

Azure/azure-rest-api-specs#3614
Azure/azure-rest-api-specs#3609
Azure/azure-rest-api-specs#3430


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.

@solankisamir
Copy link
Member Author

@shahabhijeet looks to be some issue with infra

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.

Looks great apart from the comment

Start-AutoRestCodeGeneration -ResourceProvider "apimanagement/resource-manager" -AutoRestVersion "latest" -AutoRestCodeGenerationFlags "--tag=package-2018-01"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set this tag as the default tag in the REST spec. Is there a reason why you are picking a particular tag here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we want the documentation to be generated off the -preview version, but the SDK to be targeted for specific stable version.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC documentation is generated from the published nuget package, so SDK and documentation should ideally be the same version
@shahabhijeet could you please take a look

@shahabhijeet
Copy link
Member

@solankisamir if your purpose is to generate against selective tag, then that selective tag should be the default tag in your Readme.md.
Basically you have tag1, tag2, tag3 for your various versions of spec files.
If you have decided to generate SDK against tag2, let tag2 be the default tag that all future SDKs will be generated, until you change it to something else.
You need to tie your intention to generate SDK in the rest spec repository Readme.md than generate.ps1 file in SDK repo.
Also how are JAVA, python SDKs suppose to get generate, who is keeping track as to which tag are they using?
This will cause a lot of confusion when we detect a security issue and have to regenerate and publish SDKs on a short notice.

@solankisamir
Copy link
Member Author

@dsgouda updated the SDK generation to be agnostic of version.

@solankisamir
Copy link
Member Author

@shahabhijeet @dsgouda a gentle ping on this!

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.

LGTM

@dsgouda
Copy link
Contributor

dsgouda commented Aug 16, 2018

Apologize for the delay

@dsgouda dsgouda merged commit a6ff07c into Azure:psSdkJson6 Aug 16, 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.

3 participants