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

[LUIS] Adding app version settings APIs to swagger file. #3850

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

@msftclas
Copy link

msftclas commented Sep 9, 2018

CLA assistant check
All CLA requirements met.

@AutorestCI
Copy link

AutorestCI commented Sep 9, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Sep 9, 2018

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@AutorestCI
Copy link

AutorestCI commented Sep 9, 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 9, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI
Copy link

AutorestCI commented Sep 9, 2018

Automation for azure-sdk-for-go

Encountered an unknown error: (azure-sdk-for-go)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 33, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 170, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 185, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 151, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 309, in generate_sdk_from_git_object
    sdk_repo.git.push('origin', base_branch, set_upstream=True)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 548, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1014, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 825, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(1)
  cmdline: git push --set-upstream origin restapi_auto_cognitiveservices/data-plane/LUIS/Authoring
  stderr: 'To https://github.com/Azure/azure-sdk-for-go.git
 ! [rejected]          restapi_auto_cognitiveservices/data-plane/LUIS/Authoring -> restapi_auto_cognitiveservices/data-plane/LUIS/Authoring (fetch first)
error: failed to push some refs to 'https://AutorestCI:[email protected]/Azure/azure-sdk-for-go.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.'

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@mohabghanem
Copy link
Contributor

@yangyuan Please have a look at this PR

@hovsepm
Copy link
Contributor

hovsepm commented Sep 10, 2018

@LanaShafik what is the difference between VersionSettingObject and VersionSettingUpdateObject models? they seems to have the same fields. Can they be merged into one?

@LanaShafik
Copy link
Contributor Author

LanaShafik commented Sep 12, 2018

@hovsepm
They are the two different objects in the code base:

public class AppVersionSettingObject
    {
        [JsonIgnore]
        public Guid AppVersionId { get; set; }

        [JsonProperty("name")]
        public string Key { get; set; }

        [JsonProperty("value")]
      }
public class SettingUpdateObject
    {
        [JsonProperty("name")]
        public string Name { get; set; }

        [JsonProperty("value")]
        public string Value { get; set; }
    }

So, one contains the app version id while the other does not.

@hovsepm
Copy link
Contributor

hovsepm commented Sep 12, 2018

@LanaShafik my question is not about AppVersionSettingObject object. I'm asking about VersionSettingObject and VersionSettingUpdateObject objects. What are their differences?

@hovsepm
Copy link
Contributor

hovsepm commented Sep 12, 2018

@LanaShafik here is what your edits contain in the swagger:

    "VersionSettingObject": {
      "description": "Object model of an application version setting.",
      "type": "object",
      "properties": {
        "name": {
          "description": "The application version setting name.",
          "type": "string"
        },
        "value": {
          "description": "The application version setting value.",
          "type": "string"
        }
      }
    },
    "VersionSettingUpdateObject": {
      "description": "Object model of an application version updated setting.",
      "type": "object",
      "properties": {
        "name": {
          "description": "The application version setting name.",
          "type": "string"
        },
        "value": {
          "description": "The application version updated setting value.",
          "type": "string"
        }
      }
    }

there is no such object as AppVersionSettingObject.

@mohabghanem
Copy link
Contributor

@hovsepm These two objects serve different purposes. They may currently have the same fields, since they are part of a relatively new feature. But later on as features get added, their attributes will differ. I personally think it is better to have them decoupled as two separate objects from the beginning.

@hovsepm
Copy link
Contributor

hovsepm commented Sep 13, 2018

@mohabghanem I need more explanation. What do you mean by their attributes will differ? If there is a filed that you want to stay read-only (i.e. clients will not send it back to server) you should rather mark those fields as "readOnly": true. Otherwise please explain what will be different in these two objects in the future.

According to your swagger changes VersionSettingObject will be used in Settings_List method and VersionSettingUpdateObject will be used in Settings_Update method. What I read from this is that you will have List and Update calls to the server from the client. Now from the developer point of view to accomplish some kind of their scenario they will have to List all the settings (through List) and may need to update some of them. And now for update after list call they will have to copy all the fields from the object retrieved from List call, modify whatever they need and send a new object to Server through Update call. I do not think forcing developers to implement Clone functionality for list/update scenario is a good approach.

@LanaShafik LanaShafik force-pushed the t-lasha/LUIS_appVersion_settings_APIs_addition branch from 385b638 to 4311978 Compare September 17, 2018 10:27
@LanaShafik
Copy link
Contributor Author

@hovsepm
please check the updates.

@hovsepm hovsepm merged commit f23d6ff into Azure:master Sep 17, 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.

7 participants