Skip to content

Conversation

@mohabghanem
Copy link
Contributor

@mohabghanem mohabghanem commented Aug 6, 2018

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 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.

@dsgouda
Copy link
Contributor

dsgouda commented Aug 7, 2018

@mohabghanem please link the related REST spec PR

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change. Please bump the major version number here Also, update the PackageReleaseNotes accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the AssemblyInfo here based on these version updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsgouda where to update the PackageReleaseNotes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the AssemblyFileVersion too

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the PackageReleaseNotes here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsgouda what should be in the PackageReleaseNotes ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What changes you are planning to release in this version. Please check the
"Changes in this release:" section

@mohabghanem
Copy link
Contributor Author

@dsgouda please review

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 a minor comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

PackageReleaseNotes must be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsgouda I edited the package release notes. But I do not know what else to add, because the only change in this PR is customizing the endpoint

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 7, 2018

Will merge on CIs passing

Copy link
Member

Choose a reason for hiding this comment

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

Suggest to use below description: (The reason is we may not change Azure Portal quickly, so better in here (as well as documents) we emphasize that the Endpoint should only contains protocol and hostname)

  1. Supported customizing service endpoints by assigning the endpoint string to LUISRuntimeClient.Endpoint. The endpoint string can be found on Azure Portal, it should contain only protocol and hostname, for example: https://westus.api.cognitive.microsoft.com.
    2....

@mohabghanem
Copy link
Contributor Author

@dsgouda @yangyuan I get the build error 'Build step 'Build a Visual Studio project or solution using MSBuild' marked build as failure' from the automated build. However builds and tests ran successfully on my local machine before pushing. Any clues on what may be causing this problem it?

Copy link
Member

Choose a reason for hiding this comment

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

TTypo :)

Copy link
Member

Choose a reason for hiding this comment

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

For Authoring. The client name should be LUISAuthoringClient

@yangyuan
Copy link
Member

yangyuan commented Aug 8, 2018

@mohabghanem I have no clue about that build error.
Can you try rebase to latest Azure:psSdkJson6? If CI do merge before build, then the error might be cause by recent psSdkJson6 changes.

@mohabghanem
Copy link
Contributor Author

@yangyuan @dsgouda The tests are now passing. Please review.

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 dsgouda merged commit fa8ec37 into Azure:psSdkJson6 Aug 9, 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