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

Removes M365 group and connected site. Closes #5224 #5409

Closed
wants to merge 2 commits into from

Conversation

Saurabh7019
Copy link
Contributor

Closes #5224

@Saurabh7019 Saurabh7019 marked this pull request as ready for review August 22, 2023 08:16
@milanholemans
Copy link
Contributor

Thank you @Saurabh7019, we'll try to review it ASAP!

@milanholemans
Copy link
Contributor

Could you resolve the merge conflicts please @Saurabh7019?

@milanholemans milanholemans marked this pull request as draft August 31, 2023 16:07
@Saurabh7019
Copy link
Contributor Author

Oh! I did git rebase main to resolve the conflicts and now all my changes are gone. :( Is there a way to recover the lost changes? @milanholemans

@milanholemans
Copy link
Contributor

Hi @Saurabh7019, is this the commit you are looking for? 5fdba36

If so, I suggest that you run:

  • git reset main --hard (start again from the main branch)
  • git cherry-pick 5fdba369cb261ed46e0e9889959125606495ea2c and resolve merge conflicts if there are any (apply the specific commit back to the branch)
  • git push --force (rewrite history)

Hopefully that will do it 🤞

@Saurabh7019
Copy link
Contributor Author

Thank you for saving me a lot of time; I will update this PR tonight!

@Saurabh7019 Saurabh7019 marked this pull request as ready for review September 6, 2023 16:52
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Nice start. Let's do some adjustments before we continue

src/m365/aad/commands/m365group/m365group-remove.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/m365group/m365group-remove.spec.ts Outdated Show resolved Hide resolved
@Saurabh7019
Copy link
Contributor Author

I am unable to drop the 'Empty commit' that I added to re-trigger the build pipeline. For now, I will mark the PR as 'Ready for review' and attempt to remove the empty commit later.

@Saurabh7019 Saurabh7019 marked this pull request as ready for review September 18, 2023 09:46
@Adam-it
Copy link
Member

Adam-it commented Sep 19, 2023

@waldekmastykarz I saw you already started this one. Do you want to continue?

Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Haven't done a full review, just noticed something we can improve.

src/utils/timeUtil.ts Outdated Show resolved Hide resolved
@Saurabh7019
Copy link
Contributor Author

Haven't done a full review, just noticed something we can improve.

I have removed the timeUtil and implemented the suggested change. Additionally, I have updated my previous email on the contributors list to reflect my current status as part of this PR. I hope that is okay.

@Adam-it
Copy link
Member

Adam-it commented Oct 9, 2023

@Saurabh7019 sorry for the hold up. Do you think you could rebase before we proceed?

@Adam-it Adam-it self-assigned this Oct 9, 2023
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

tested locally ✅
works nicely 👍
LGTM 👍
you rock 👏

@Adam-it
Copy link
Member

Adam-it commented Oct 15, 2023

ready to merge 🚀

@Adam-it Adam-it added pr-merged hacktoberfest-accepted Accept for hacktoberfest, will merge later labels Oct 15, 2023
@Adam-it
Copy link
Member

Adam-it commented Oct 15, 2023

merged manually. Awesome work 👏

@Adam-it Adam-it closed this Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later pr-bugfix pr-merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug report - 'aad o365group remove' - the connected SharePoint site is not removed
4 participants