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

husamhilal review #1 of Networking Guide #259

Merged
merged 9 commits into from
May 3, 2023
Merged

Conversation

husamhilal
Copy link
Contributor

Overview/Summary

Replace this with a brief description of what this Pull Request fixes, changes, etc.

This PR fixes/adds/changes/removes

  1. Replace me
  2. Replace me
  3. Replace me

Breaking Changes

  1. Replace me
  2. Replace me

Testing Evidence

  • Replace this with any testing evidence to show that your Pull Request works/fixes as described and planned (include screenshots, if appropriate)

As part of this Pull Request I have

  • Checked for duplicate Pull Requests
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Performed testing and provided evidence.
  • Updated relevant and associated documentation.

@fguerri @sblair01 @prasad3017 

-bullet #4 at explaining Figure 1 - My question is:
Leveraging DNAT and SNAT rules at NSX-T cannot we control if Public IP can be used only for Outbound but not for Inbound?
Looks great! mainly formatting changes!
Looks great! Thanks @fguerri!

I just made few formatting changes.

@fguerri might be helpful to add a note describing the difference between ExpressRoute “Private” peering and ExpressRoute “Microsoft” peering to give reader a quick glance on the main difference.
In the diagram, it says "Read section: Connectivity with Azure VNet when ER Transit not used"... would probably make sense to actually point to that explicitly in the documentation page. Also, there is no specific paragraph talking about UDR (following the diagram) in that section.
@fguerri @prasad3017  @sblair01
added note about Application Gateway and Azure firewalls.
+ minor formatting changes

@fguerri
@husamhilal husamhilal added the documentation Improvements or additions to documentation label May 1, 2023
Copy link
Member

@fguerri fguerri left a comment

Choose a reason for hiding this comment

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

Thank you @husamhilal for reviewing. Please see my observations in the comments and let me know what you think.

Network Design Guide/avs-networking-basics.md Outdated Show resolved Hide resolved
Network Design Guide/avs-networking-basics.md Show resolved Hide resolved
Network Design Guide/avs-networking-basics.md Show resolved Hide resolved
Network Design Guide/avs-networking-basics.md Outdated Show resolved Hide resolved
Network Design Guide/readme.md Outdated Show resolved Hide resolved
Network Design Guide/readme.md Outdated Show resolved Hide resolved
@husamhilal
Copy link
Contributor Author

Thank you @husamhilal for reviewing. Please see my observations in the comments and let me know what you think.

Thanks a lot @fguerri for the feedback. I addressed the points you mentioned. So, you can merge if you'd like to. I also added comments to my initial commits, not sure if you got those as feedback for further enhancements.

@fguerri
Copy link
Member

fguerri commented May 3, 2023

I have not gone through the other comments yet. I will do. For now, merging the changes that you suggested in this PR.

@fguerri fguerri merged commit e98c9a7 into main May 3, 2023
@husamhilal
Copy link
Contributor Author

I have not gone through the other comments yet. I will do. For now, merging the changes that you suggested in this PR.

Sounds a great plan. Thanks @fguerri 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants