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

Modifying app version setting name and Removing region from publish app. #3930

Conversation

LanaShafik
Copy link
Contributor

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

  • The title of the PR 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 information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@AutorestCI
Copy link

AutorestCI commented Sep 18, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI
Copy link

AutorestCI commented Sep 18, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Sep 18, 2018

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2751

@AutorestCI
Copy link

AutorestCI commented Sep 18, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#3362

@AutorestCI
Copy link

AutorestCI commented Sep 18, 2018

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@LanaShafik LanaShafik force-pushed the t-lasha/LUIS_appVersion_settings_modifications branch from c6165ba to 095bb5e Compare September 18, 2018 18:23
@lmazuel
Copy link
Member

lmazuel commented Sep 18, 2018

Hi @LanaShafik
The title of the PR does not match what I see inside. Could you clarify?
Also, you are remving a field, was it here by mistake and never populated? Or did you removed it from the server? Could you give me more context, since this is technically a breaking change that will have to be documented in releases of SDK.
Thank you!

@@ -8224,10 +8224,6 @@
"description": "Indicates if the staging slot should be used, instead of the Production one.",
"default": false,
"type": "boolean"
},
Copy link
Member

Choose a reason for hiding this comment

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

So the LUIS app is now cross-region?

@LanaShafik
Copy link
Contributor Author

Hi @lmazuel

I only changed the "UseNegativeSampling" setting to "UseAllTrainingData".
@mohabghanem will give you more information about the other changes. I will change the PR title to be more convenient.

@LanaShafik LanaShafik changed the title Modifying app version setting name. Modifying app version setting name and Removing region from publish app. Sep 19, 2018
@mohabghanem
Copy link
Contributor

mohabghanem commented Sep 19, 2018

@lmazuel @yangyuan concerning the change in the region field. Previously, the user used to choose the region they want to publish their app in, so the region field was sent by the publish request. But now when the user issues a publish request, we automatically publish their app to every region they are subscribed to, so the region field is not used anymore.
I know this change is irrelevant to the main purpose of the PR, but I think it is too small to have a PR on its own, so we brought it along.

@lmazuel
Copy link
Member

lmazuel commented Sep 19, 2018

Hi @mohabghanem
I think atomic PR (i.e. one PR for one purpose) is a good habit, and would have been faster here, since I wouldn't had to ask for more context :). There is no such thing as "small PR", since a PR that contains things that are not described in the title just makes me feel someone made a mistake and didn't intended the commit.
Thanks for the additional context! Merging the PR, but that's an important breaking change for SDK: but it's not moving from "required" to "optional", it disappears completely from the method signature. For instance, my Python sample is wrong now.

@lmazuel lmazuel merged commit f76c5a7 into Azure:master Sep 19, 2018
@AutorestCI
Copy link

AutorestCI commented Sep 19, 2018

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

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