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

Add AzureFirewall swagger and examples #3276

Merged
merged 2 commits into from
Jun 25, 2018
Merged

Add AzureFirewall swagger and examples #3276

merged 2 commits into from
Jun 25, 2018

Conversation

joannakleong
Copy link
Contributor

@joannakleong joannakleong commented Jun 20, 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

@joannakleong joannakleong changed the title Add AzureFirewall swagger and examples [do not merge]Add AzureFirewall swagger and examples Jun 20, 2018
@AutorestCI
Copy link

AutorestCI commented Jun 20, 2018

Automation for azure-sdk-for-node

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-node#2610

@AutorestCI
Copy link

AutorestCI commented Jun 20, 2018

Automation for azure-sdk-for-java

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-java#2135

@AutorestCI
Copy link

AutorestCI commented Jun 20, 2018

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#2944

@AutorestCI
Copy link

AutorestCI commented Jun 20, 2018

Automation for azure-sdk-for-ruby

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

@sergey-shandar sergey-shandar added DoNotMerge <valid label in PR review process> use to hold merge after approval WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jun 20, 2018
@AutorestCI
Copy link

AutorestCI commented Jun 20, 2018

Automation for azure-sdk-for-go

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

@sergey-shandar
Copy link
Contributor

@ravbhatnagar new spec AzureFirewall.

@sergey-shandar sergey-shandar requested a review from dsgouda June 20, 2018 18:41
@joannakleong
Copy link
Contributor Author

Hi @ravbhatnagar , @sergey-shandar , FYI we had gotten ARM approval when we prepared for private preview (https://github.com/Azure/azure-rest-api-specs-pr/pull/246)

"properties":{
"privateIPAddress": {
"type": "string",
"description": "PrivateIPAddress of the network interface IP Configuration. This field is populated in the output."

Choose a reason for hiding this comment

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

This is actually the front end IP address of the Internal Loadbalancer. we can check with Yair on how to expose this and if we should say this is the IPAddress they need to set as nexthop in UDR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spoke with Yair, we will use the following description: The Firewall Internal Load Balancer IP to be used as the next hop in User Defined Routes

"format":"int32",
"maximum":65000,
"exclusiveMaximum":false,
"minimum":0,

Choose a reason for hiding this comment

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

We start priority from 101. 0 - 100 and > 6500 is reserved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In NRP looks like user can start from 100. Will update the minimum to reflect that. Thanks for catching

@dsgouda
Copy link
Contributor

dsgouda commented Jun 21, 2018

@MikhailTryakhov
Can you take a look at this PR. Is this orthogonal to the changes here
Will trust @sergey-shandar 's review if Mikhail is okay with this change

@sergey-shandar sergey-shandar removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 21, 2018
Copy link
Contributor

@MikhailTryakhov MikhailTryakhov left a comment

Choose a reason for hiding this comment

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

LGTM. It's a new feature, Samer approved to push it to master directly.
Please merge Network-2018-06-01 first and this one after

},
"description":"Collection of rules used by a network rule collection."
},
"provisioningState":{

Choose a reason for hiding this comment

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

We only care about provisioning state on AzureFirewallProperties why do we have to mentioned provisioning state on these child resources? we don't have operations on them. Also the same is for Ipconfig as well.

Choose a reason for hiding this comment

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

I see that other resources have it and NRP returns this value. So you can ignore this comment.

@sergey-shandar
Copy link
Contributor

@joannakleong follow @MikhailTryakhov advise and then let me know when it can be merged.

@dsgouda
Copy link
Contributor

dsgouda commented Jun 21, 2018

@sergey-shandar This is good to merge

@joannakleong
Copy link
Contributor Author

Thanks everyone for the prompt review! @sergey-shandar Before merging, I'd like to resolve @MadhusudhanRavi comments on the description fields for a couple properties

@joannakleong
Copy link
Contributor Author

@sergey-shandar Pushed a final commit. Can this swagger be merged next Tuesday?

@dsgouda
Copy link
Contributor

dsgouda commented Jun 25, 2018

@sergey-shandar This good to merge. The dependent PR here has been merged
Please consult with @lmazuel regarding the CI failures.

@sergey-shandar sergey-shandar removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label Jun 25, 2018
@sergey-shandar
Copy link
Contributor

I spoke with @lmazuel, it's a timeout issue.

@lmazuel lmazuel merged commit 32b0d87 into Azure:master Jun 25, 2018
@MikhailTryakhov MikhailTryakhov changed the title [do not merge]Add AzureFirewall swagger and examples Add AzureFirewall swagger and examples Jun 28, 2018
@sergey-shandar
Copy link
Contributor

@joannakleong Could you send me a link to ARM approval? This link https://github.com/Azure/azure-rest-api-specs-pr/pull/246 has no AzureFirewall.

@joannakleong
Copy link
Contributor Author

@sergey-shandar yes, this is because for private preview we named ourselves "Secure Gateway" and now we've finalized the name "Azure Firewall"

@lmazuel
Copy link
Member

lmazuel commented Jul 17, 2018

@AutorestCI rebuild azure-sdk-for-python

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