Skip to content

Conversation

@yuncmsft
Copy link

@yuncmsft yuncmsft commented Jul 31, 2018

need to add three location tabs to heatmap bottom view port (currently one table showing all source IPs/query counts). * discussion and feedback needed *

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

need to add three location tabs to heatmap bottom view port (currently one table showing all source IPs/query counts).
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Jul 31, 2018

Automation for azure-sdk-for-python

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

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 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  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(128)
  cmdline: git checkout c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f
  stderr: 'fatal: reference is not a tree: c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f'

@AutorestCI
Copy link

AutorestCI commented Jul 31, 2018

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#3275

@AutorestCI
Copy link

AutorestCI commented Jul 31, 2018

Automation for azure-sdk-for-ruby

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

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 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  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(128)
  cmdline: git checkout c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f
  stderr: 'fatal: reference is not a tree: c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f'

@AutorestCI
Copy link

AutorestCI commented Jul 31, 2018

Automation for azure-sdk-for-java

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

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 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  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(128)
  cmdline: git checkout c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f
  stderr: 'fatal: reference is not a tree: c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f'

@AutorestCI
Copy link

AutorestCI commented Jul 31, 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 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  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(128)
  cmdline: git checkout c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f
  stderr: 'fatal: reference is not a tree: c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f'

[change type to number] 
Need to add three location tabs under current Traffic Manager Heatmap view port.  [discussion and feedback needed]
in addition to default heatmap table, add three location options
@msftclas
Copy link

msftclas commented Aug 1, 2018

CLA assistant check
All CLA requirements met.

"required": true,
"type": "string",
"enum": [
"default"
Copy link
Member

Choose a reason for hiding this comment

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

Missing comma

missing comma for heatMapType enum.
yuncmsft added a commit to yuncmsft/azure-rest-api-specs that referenced this pull request Aug 1, 2018
an JSON example to reuse heatmap for heatMapLocation: [Azure/azure-rest-api-specs] add location tabs to heatmap bottom view port (Azure#3559)
add HeatMapPropertyBase and four sub-classes: default, asn, country, state;
add links to JSON examples
update HeatMapModel
example of HeatMapCountry GET (heapMapType == "country")
example for HeatMapASN GET responses (heatMapType =="asn")
an example for HeatMapState GET response (heatMapType == "state")
"heatMapType"
],
},
"default":{
Copy link
Member

Choose a reason for hiding this comment

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

Use a meaingful name, and the x-ms-discriminator-value for the real value. Example:

    "DatasetBZip2Compression": {
      "x-ms-discriminator-value": "BZip2",

This means "Create class DatasetBZip2Compression, but match value BZip2 on the wire"

Copy link
Author

Choose a reason for hiding this comment

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

I see. so here I can use
"HeatMapPropertyAsn":
{"x-ms-discriminator-value": "asn",
to indicate that when heatMapType=="asn", the HeatMapPropertyAsn will be applied?
if so, that's very convenient - I was very confused how I can link the two.

}
]
},
"asn":{
Copy link
Member

Choose a reason for hiding this comment

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

Use a meaingful name, and the x-ms-discriminator-value for the real value. Example:

    "DatasetBZip2Compression": {
      "x-ms-discriminator-value": "BZip2",

This means "Create class DatasetBZip2Compression, but match value BZip2 on the wire"

}
]
},
"country":{
Copy link
Member

@lmazuel lmazuel Aug 3, 2018

Choose a reason for hiding this comment

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

Use a meaningful name, and the x-ms-discriminator-value for the real value. Example:

    "DatasetBZip2Compression": {
      "x-ms-discriminator-value": "BZip2",

This means "Create class DatasetBZip2Compression, but match value BZip2 on the wire"

},
"required": [
"heatMapType"
],
Copy link
Member

Choose a reason for hiding this comment

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

Extra comma, invalid JSON

"description": "Class representing a Traffic Manager HeatMap properties."
]
},
"state":{
Copy link
Member

Choose a reason for hiding this comment

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

Use a meaningful name, and the x-ms-discriminator-value for the real value. Example:

    "DatasetBZip2Compression": {
      "x-ms-discriminator-value": "BZip2",

This means "Create class DatasetBZip2Compression, but match value BZip2 on the wire"

},
"description": "Class which is a sparse representation of a Traffic Manager endpoint."
},
"HeatMapLocation": {
Copy link
Member

Choose a reason for hiding this comment

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

Now you split everything, this type should be used somewhere.

Copy link
Author

Choose a reason for hiding this comment

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

ar... forgot to update this def name, should be "TrafficLocation" - i'm assuming when I use "$ref": "#/definitions/TrafficLocation", this "TrafficLocation" will be applied.

"default" -> HeatMapProperties
"asn" -> HeatMapPropertyAsn
"country" -> HeatMapPropertyCountry
"state" -> HeatMapPropertyState
rename HeatMapLocation to TrafficLocation (referenced)
@lmazuel lmazuel requested a review from ravbhatnagar August 3, 2018 23:06
@lmazuel lmazuel added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 3, 2018
update resource type name
update resource type
update resource type
update discriminator value to be resource type
Copy link
Contributor

@ravbhatnagar ravbhatnagar left a comment

Choose a reason for hiding this comment

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

some noticeable issues. Lets close over a quick call. I want to make sure service is doing the right thing here.

"responses": {
"200": {
"body": {
"id": "/subscriptions/{subscription-id}/resourceGroups/azuresdkfornetautoresttrafficmanager1323/providers/Microsoft.Network/trafficManagerProfiles/azuresdkfornetautoresttrafficmanager3880/heatMaps/stateTrafficFraction",
Copy link
Contributor

Choose a reason for hiding this comment

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

name value coming from "id" does not match the value in "name" property. Please make sure service returns the correct "id" and "name" value

Copy link
Author

Choose a reason for hiding this comment

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

updated name and type - do we need a separate API version?

"body": {
"id": "/subscriptions/{subscription-id}/resourceGroups/azuresdkfornetautoresttrafficmanager1323/providers/Microsoft.Network/trafficManagerProfiles/azuresdkfornetautoresttrafficmanager3880/heatMaps/countryTrafficFraction",
"name": "country",
"type": "Microsoft.Network/trafficManagerProfiles/heatMaps/countryTrafficFraction",
Copy link
Contributor

Choose a reason for hiding this comment

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

name, id, type values are all messed up. Please set up a sync with armapireview to get this approved.

@ravbhatnagar
Copy link
Contributor

LOoks like you have decided to go with enum for heatmap type with fixed set of names that can be given instead of $expand. Looks fine to me.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 15, 2018
"HeatMapProperties": {
"HeatMapPropertyBase": {
"type": "object",
"discriminator": "heatMapType",
Copy link
Member

Choose a reason for hiding this comment

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

That's not exactly the way to do it, but we're close :)

  • HeatMapModel should declare id, name, and type and nothing more
  • HeatMapModel should have the discriminator on "type", since this is the actual value that discriminates (heatMapType does not exist in the JSON).
  • For each type, you need two classes (take Country as example)
    • HeatMapModelCountry that is a allOf of HeatMapModel and has the x-discriminator-value, and contains one node properties described in a class HeatMapModelCountryProperties. This last one does not talk about discriminator

Hope this clarify a bit, it's easier to see now we made progress and have examples.

yuncmsft and others added 5 commits August 15, 2018 10:52
- Include ARM's comments to update name, and type. 
- Swagger discriminator now uses 'name' as distinguisher:  "default", "asnTrafficFraction", "countryTrafficFraction", "stateTrafficFraction".
- Updated existing HeatMapModel to be the super class with four dervatives with a discriminator value in each class: HeatMapModelDefault, HeatMaptModelAsn, HeatMapModelCountry, HeatMapModelState. 
- Each sub-model has a corresponding properties class
remove extra comma
@azuresdkci azuresdkci assigned dsgouda and unassigned lmazuel Dec 22, 2018
@azuresdkci azuresdkci requested a review from dsgouda December 22, 2018 00:33
@salameer
Copy link
Member

salameer commented Apr 4, 2019

@lmazuel do you recall what's the deal with this one? should it be merged?

@salameer
Copy link
Member

salameer commented Apr 5, 2019

ping @lmazuel :)

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 good for the most part

"description": "Class which is a sparse representation of a Traffic Manager endpoint."
},
"TrafficLocation": {
"properties": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mark the readonly properties wherever applicable

@salameer
Copy link
Member

@yuncmsft can you please address the changes requested?

@salameer
Copy link
Member

CLosing since there's no response from @yuncmsft please request to reopen if required

@salameer salameer closed this Apr 23, 2019
@AutorestCI
Copy link

AutorestCI commented Apr 23, 2019

Automation for azure-sdk-for-js

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

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 254, in generate_sdk_from_git_object
    with manage_git_folder(gh_token, Path(temp_dir) / Path("rest"), branched_rest_api_id, pr_number=pr_number) as restapi_git_folder, \
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 272, in manage_git_folder
    clone_to_path(gh_token, temp_dir, split_git_id[0], branch_or_commit=branch, pr_number=pr_number)
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 212, in clone_to_path
    repo.git.checkout(branch_or_commit)
  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(128)
  cmdline: git checkout c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f
  stderr: 'fatal: reference is not a tree: c1b4577f9fec6f9274e5a7b9d7c026898f04ce8f'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants