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.

Sign up for GitHub

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

Jump to bottom

docs: Added new recommendations for Azure bastion hosts #586

Merged
merged 6 commits into from
Jan 16, 2025
Merged

docs: Added new recommendations for Azure bastion hosts #586

merged 6 commits into from
Jan 16, 2025

Conversation

sonmitt
Copy link
Contributor

@sonmitt sonmitt commented Dec 18, 2024 •
edited
Loading

Overview/Summary

Added new recommendations for Azure bastion hosts and cross referenced them in dependent resource section in Specialized workload/AI

Related Issues/Work Items

https://dev.azure.com/CSUSolEng/Accelerator%20-%20AI/_workitems/edit/38508/

As part of this pull request I have

  • Read the Contribution Guide and ensured this PR is compliant with the guide
  • Checked for duplicate Pull Requests
  • Associated it with relevant GitHub Issues or ADO Work Items (Internal Only)
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Ensured PR tests are passing
  • Performed testing and provided evidence (e.g. screenshot of output) for any changes associated to ARG queries
  • Updated relevant and associated documentation (e.g. Contribution Guide, Docs etc.)

@Copilot Copilot bot review requested due to automatic review settings December 18, 2024 11:33
@sonmitt sonmitt requested review from a team as code owners December 18, 2024 11:33
Copilot
Copilot AI reviewed Dec 18, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
rodrigosantosms
rodrigosantosms requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@rodrigosantosms rodrigosantosms left a comment

Choose a reason for hiding this comment

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

  • Recommendation: "Deploy Azure Bastion across Availability Zones"
    I suggest we move this recommendation from High to Medium, since Bastion is not directly used by workloads, it is an important component that allows admins to access their environments for troubleshooting, but deploying a bastion in a zone redundant config would not directly increase the resilience of the workload.

For example, if they do not deploy a Load Balancer across zones and a Zone goes down, it will have direct effect on the workload, while Bastion would not.

@oZakari oZakari added Enhancement 🆕 New feature or request Needs: Author Feedback 👂 The issue or pull request needs feedback from the original author labels Dec 18, 2024
@sonmitt
Copy link
Contributor Author

sonmitt commented Dec 19, 2024

Thanks Rodrigo. Make sense to make the impact as Medium. I will do the changes.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 Needs attention from aprl-maintainers and removed Needs: Author Feedback 👂 The issue or pull request needs feedback from the original author labels Dec 19, 2024
@sonmitt
Copy link
Contributor Author

sonmitt commented Dec 19, 2024

Review comment has been addressed.

oZakari
oZakari approved these changes Jan 6, 2025
View reviewed changes
Copy link
Collaborator

@oZakari oZakari left a comment

Choose a reason for hiding this comment

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

LGTM

@oZakari oZakari added the Needs: Attention from aprl-maintainers 👋 Needs attention from aprl-admins GitHub team. label Jan 6, 2025
@sonmitt
Copy link
Contributor Author

sonmitt commented Jan 13, 2025

@rodrigosantosms, proposed changes have been done. Please approve the PR.

rodrigosantosms
rodrigosantosms approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@rodrigosantosms rodrigosantosms left a comment

Choose a reason for hiding this comment

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

lgtm

@ejhenry
Copy link
Contributor

ejhenry commented Jan 15, 2025

@sonmitt please resolve the conflicts

@ejhenry ejhenry added the Needs: Author Feedback 👂 The issue or pull request needs feedback from the original author label Jan 15, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs: Author Feedback 👂 The issue or pull request needs feedback from the original author label Jan 15, 2025
@sonmitt
Copy link
Contributor Author

sonmitt commented Jan 15, 2025

@oZakari and @ejhenry, I resolved the merge conflicts. Looks like it need approvals again.

ejhenry
ejhenry approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@ejhenry ejhenry left a comment

Choose a reason for hiding this comment

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

LGTM

@ejhenry ejhenry merged commit ade9403 into Azure:main Jan 16, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@ejhenry ejhenry ejhenry approved these changes

@oZakari oZakari oZakari approved these changes

@rodrigosantosms rodrigosantosms rodrigosantosms approved these changes

Assignees
No one assigned
Labels
Enhancement 🆕 New feature or request Needs: Attention from aprl-maintainers 👋 Needs attention from aprl-admins GitHub team. Needs: Attention 👋 Needs attention from aprl-maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sonmitt @ejhenry @rodrigosantosms @oZakari