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 AAD hybrid auth workbook #1001

Merged
merged 7 commits into from Nov 3, 2020
Merged

Conversation

MichelleSwafford
Copy link
Contributor

This is a hybrid auth workbook that gives customers visibility into add/remove users and groups from staged rollout and also tracks state of sign-ins for users in staged rollout

PR Checklist

  • Explain your changes, so people looking at the PR know what and why, the code changes are the how.
  • if updating templates, it is helpful to post a screenshot of what the changes look like.
  • if updating templates, ensure that your parameters and steps have meaningful names.
  • if adding new templates, ensure that your parameters and columns have display names.
  • if adding new templates, or changing items in a gallery, it's helpful to post a screenshot of what the gallery looks like now

This is a hybrid auth workbook that gives customers visibility into add/remove users and groups from staged rollout and also tracks state of sign-ins for users in staged rollout
@MichelleSwafford MichelleSwafford requested a review from a team as a code owner October 27, 2020 16:59
@MichelleSwafford
Copy link
Contributor Author

This is a hybrid auth workbook that gives customers visibility into add/remove users and groups from staged rollout and also tracks state of sign-ins for users in staged rollout

@gardnerjr
Copy link
Contributor

because this is in a new top level folder, it doesn't automatically show up as owned by the AAD workbooks team. you probably want to move this from
Workbooks/Azure Active Directory Hybrid Auth Staged Rollout/Hybrid Auth Staged Rollout/StagedRollout.workbooks.txt

to one folder inside of the existing Azure Active Directory folder?

Workbooks/Azure Active Directory/Hybrid Auth Staged Rollout/Hybrid Auth Staged Rollout/StagedRollout.workbooks.txt

@gardnerjr gardnerjr changed the title Add files via upload Add AAD hybrid auth workbook Oct 27, 2020
@gardnerjr
Copy link
Contributor

Also, it looks like you previously had a PR open to do the same thing, and closed it and made a new one? you can keep making changes to an existing PR while in progress, you don't need to close the other one and start over (if you do, people won't see the history of previous comments in the PR, etc)

@MichelleSwafford
Copy link
Contributor Author

I think that Hybrid auth should have its own category. I'm planning to add more workbooks under that category like workbooks for PTA.

@gardnerjr
Copy link
Contributor

that's fine then, but you'll also need to add a line to the CODEOWNERS file (by the existing ones) that assign code owners to your new top level folder then. otherwise my team is required to approve things instead of your team.

@MichelleSwafford
Copy link
Contributor Author

Can this be checked-in?

@gardnerjr
Copy link
Contributor

has anyone from AAD/your team reviewed this to approve it? Again, workbooks team can only verify for some things, we don't know how your features work or what to expect inside the template when it runs. There aren't any screenshots, examples, etc, and we might not have access to the data so if i try to load this template, it might just be a big "no data found" for us.

@MichelleSwafford
Copy link
Contributor Author

This has been reviewed and approved by my team and management.
It'll be reviewed again by the team once I check it in as preview.
I'll remove the preview only after approval from the team.

@gardnerjr
Copy link
Contributor

@MichelleSwafford if that is true, someone from your team should literally sign off on it here as well.

ideally, you'd complete your CODEOWNERS changes PR first, which should also enforce that review by someone from that team here as well.

Copy link
Contributor

@danielwood95 danielwood95 left a comment

Choose a reason for hiding this comment

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

Approving changes. Michelle fixed isPreview and made lowercase

@gardnerjr
Copy link
Contributor

can you complete the codeowners PR #1002 first, or make that change here as well? otherwise, this one still shows up as owned by our team for some amount of time until/if that PR is ever merged.

@danielwood95
Copy link
Contributor

can you complete the codeowners PR #1002 first, or make that change here as well? otherwise, this one still shows up as owned by our team for some amount of time until/if that PR is ever merged.

I've closed PR #1002 since Michelle is already part of @azure-ad-workbooks and doesn't need to be individually added to CODEOWNERS

@gardnerjr
Copy link
Contributor

@danielwood95 except that as i keep saying, this PR does not contain CODEOWNERS changes, so without #1002, there's no ownership of this folder. either this pr needs codeowners changes or that pr needs to be fixed+completed. also sending mail to clarify.

@gardnerjr gardnerjr requested a review from a team November 3, 2020 22:53
@gardnerjr
Copy link
Contributor

(what's strange is that in one place it says it is waiting for a code owner approver, but Daniel has approved it so the checks here are all green at the bottom)

@MichelleSwafford MichelleSwafford merged commit 8388442 into master Nov 3, 2020
@MichelleSwafford MichelleSwafford deleted the MichelleSwafford-patch-2 branch November 4, 2020 01:32
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.

None yet

3 participants