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

Release of Azure Networking Network-2018-06-01 (api-version 2018-04-01) #3261

Merged
merged 9 commits into from
Jun 21, 2018

Conversation

MikhailTryakhov
Copy link
Contributor

@MikhailTryakhov MikhailTryakhov commented Jun 18, 2018

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

"enum": [
"Http",
"Tcp",
"Https"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

breaking

@AutorestCI
Copy link

AutorestCI commented Jun 18, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Jun 18, 2018

Automation for azure-sdk-for-java

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

@AutorestCI
Copy link

AutorestCI commented Jun 18, 2018

Automation for azure-sdk-for-node

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

@AutorestCI
Copy link

AutorestCI commented Jun 19, 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/swaggertosdk/github_tools.py", line 32, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 167, 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 182, 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 300, 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_network/resource-manager
  stderr: 'To https://AutorestCI:58ab395c311d1bd75b3e1db1cc8adaf9acc42afe@github.com/Azure/azure-sdk-for-go.git
 ! [rejected]        restapi_auto_network/resource-manager -> restapi_auto_network/resource-manager (fetch first)
error: failed to push some refs to 'https://AutorestCI:58ab395c311d1bd75b3e1db1cc8adaf9acc42afe@github.com/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.'

@AutorestCI
Copy link

AutorestCI commented Jun 19, 2018

Automation for azure-sdk-for-ruby

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

EvgenyAgafonchikov and others added 5 commits June 19, 2018 10:36
* Init 2018-06-01 (copy from 2018-05-01)

* Make VM reference in NIC readonly

* Updated version in readme
* Added new VPN protocol

* indents
* Add examples for Express Route Circuit APIs

* Fix errors in expressRouteCircuit.json validations

* Fix errors in validate examples for ExR Circuit
@@ -669,6 +669,7 @@
"NetworkInterfacePropertiesFormat": {
"properties": {
"virtualMachine": {
"readOnly": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is this from?

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.

Left a couple of comments.
@MikhailTryakhov why is there a 2018-06-01 directory in this PR?

@@ -1,6 +1,6 @@
{
"parameters" : {
"api-version" : "2017-03-30",
"api-version": "2017-03-30",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't all versions be 2018-04-01 now? Please update all the examples here accordingly

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 as I know we shouldn't update VMSS

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that? In that case, the examples shouldn't be a part of 2018-04-01 here

Copy link
Contributor Author

@MikhailTryakhov MikhailTryakhov Jun 20, 2018

Choose a reason for hiding this comment

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

@dsgouda but they are the part already from previous months... The only change we made is identity change...
+@lmazuel could you please review what's the expected VMSS examples behavior here? As I remember you told me don't touch VMSS :)

Copy link
Member

@lmazuel lmazuel Jun 20, 2018

Choose a reason for hiding this comment

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

@MikhailTryakhov @dsgouda The way VMSS in Network is designed, they have to copy the file as-is from one month to another. This implies copy the examples with it.
If we want to avoid that copy eventually, it requires a more deeper discussion on how we structure the Swagger for Network and the peculiar VMSS. I don't think that's the point of this PR, and shouldn't be addressed 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.

@lmazuel so we should leave api version as is without api-version update? Agree ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@lmazuel agree but examples that do not match to the current version is a bit moot when being tested. Definitely needs a rethink.
@MikhailTryakhov please mark this as something we need to discuss in upcoming meetings.
Won't block on this

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 done. please approve/merge this PR?

"info": {
"title": "NetworkManagementClient",
"description": "The Microsoft Azure Network management API provides a RESTful set of web services that interact with Microsoft Azure Networks service to manage your network resources. The API has entities that capture the relationship between an end user and the Microsoft Azure Networks service.",
"version": "2018-06-01"
Copy link
Contributor

Choose a reason for hiding this comment

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

If changes are being ported from 06 to 04, there shouldn't be a 2018-06-01 directory in this PR

@MikhailTryakhov MikhailTryakhov force-pushed the Port06To04 branch 3 times, most recently from b2e6f23 to 0f31156 Compare June 20, 2018 22:46
* Adding VirtualWan Swagger

* Adding the CRUD Apis for VpnConnections

* Updating VpnGateway examples

* Fixing build errors

* Addressing comments

* Addressing comments and editing VpnConnection definition and corresponding examples

* Addressing comments

* Removing enableRateLimiting property

* Addressing comments

* Changing the version to 2018-04-01 version
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 other than a couple of comments.

@@ -731,6 +730,9 @@ output-folder: $(go-sdk-folder)/services/network/mgmt/2018-02-01/network
## Suppression
``` yaml
directive:
- suppress: RequiredPropertiesMissingInResourceModel
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this approved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is the same exception for ID readonly we had 1-2 months ago

@@ -0,0 +1,2125 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

@MikhailTryakhov Was this spec reviewed?

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
Copy link
Contributor

dsgouda commented Jun 21, 2018

@MikhailTryakhov Looks good, there is a model validation error here that needs a fix too
Apologize for the turnaround.

@MikhailTryakhov
Copy link
Contributor Author

@dsgouda I'm working with @yushwang for a 2nd month to make it fixed, now have 1 error instead of 3 in VnetGW and all of them fixed in express routes.
I'm afraid we can't wait for this error fix and have to publish with it. Fix would be merged to master when we'll have it ready

@dsgouda
Copy link
Contributor

dsgouda commented Jun 21, 2018

@MikhailTryakhov normally this would be a blocker but given the time crunch I will let this pass. Please make a note at your end to fix this ASAP

@dsgouda
Copy link
Contributor

dsgouda commented Jun 21, 2018

@lmazuel would it be okay to merge this, looks like travis is failing on timeouts while generating the sdks

@MikhailTryakhov
Copy link
Contributor Author

@dsgouda added exception for this error approved by Gaurav (please see thread "VirtualNetworkGateway spec CR. Fixed 2 of 3 ARM errors")
So we've finally resolved it!

@lmazuel could you please help with merge?

@@ -31,8 +31,6 @@ openapi-type: arm
tag: package-2018-04
```

### Tag: package-2018-05
Copy link
Contributor

Choose a reason for hiding this comment

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

@MikhailTryakhov Please undo this change

@dsgouda
Copy link
Contributor

dsgouda commented Jun 21, 2018

@lmazuel Please merge since I do not have the permissions.

@dsgouda
Copy link
Contributor

dsgouda commented Jun 21, 2018

@marstr ping

@lmazuel lmazuel merged commit 64909dc into Azure:master Jun 21, 2018
@EvgenyAgafonchikov EvgenyAgafonchikov deleted the Port06To04 branch May 15, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants