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 confirmation prompts and examples for Remove- functions #174

Conversation

giuseppecampanelli
Copy link
Contributor

@giuseppecampanelli giuseppecampanelli commented May 19, 2020

Fixes #171

@msftclas
Copy link

msftclas commented May 19, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks so much for this, @themilanfan!

Really minor feedback to address, and then I'll be happy to merge this in. Thanks again, and welcome to the project!

GitHubLabels.ps1 Outdated Show resolved Hide resolved
GitHubAssignees.ps1 Show resolved Hide resolved
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Actually, just realized that you didn't add the necessary ShouldProcess commands to have any impact here.

See

if ($PSCmdlet.ShouldProcess($project, "Remove project"))

You probably need to update a bunch of tests too in order for them to pass (adding -Confirm:$false to those calls), because once this is done, all of the existing tests that make use of Remove-* commands will fail because they'll hang waiting for user confirmation.

…pt for confirmation

Also re-addded needed whitespaces
Added line-space in example
@giuseppecampanelli
Copy link
Contributor Author

@HowardWolosky thank you for the feedback, I've just committed the changes!

Copy link
Contributor Author

@giuseppecampanelli giuseppecampanelli left a comment

Choose a reason for hiding this comment

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

Completed in new commits

Copy link
Contributor Author

@giuseppecampanelli giuseppecampanelli left a comment

Choose a reason for hiding this comment

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

Completed review in commits

@giuseppecampanelli
Copy link
Contributor Author

giuseppecampanelli commented May 21, 2020

@HowardWolosky I'm not sure what's happening with the check, I reviewed all the changes you requested and thought it would resume

@HowardWolosky
Copy link
Member

@themilanfan - Thanks for the update. Will review the changes this weekend.

As for the checks, ignore them for now. I've been running into issues with the tests properly running in PR CI builds, so I still need to verify them locally before merging. I'll get to fixing the PR CI build at some point.

@HowardWolosky HowardWolosky self-assigned this May 22, 2020
@HowardWolosky HowardWolosky added the technical debt Work that was postponed by a previous change. label May 22, 2020
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Overall, this looks like a great update, thanks!
One minor documentation suggestion in this review.
Additionally, more tests have been added since you authored this PR. Would you be willing to refresh this PR with the latest changes from master, and incorporate any remaining -Confirm:$false parameters into any new instances of Remove-* methods being called in the tests?

Thanks so much!

GitHubRepositories.ps1 Outdated Show resolved Hide resolved
@giuseppecampanelli
Copy link
Contributor Author

Hi @HowardWolosky, I've added the remaining -Confirm:$false as well as updated the documentation and fixed another small issue in GitHubLabels.tests.ps1 as well. I will also wait for this to merge so that for issue #176 I don't miss any Confirm:$false there.

Copy link
Contributor Author

@giuseppecampanelli giuseppecampanelli left a comment

Choose a reason for hiding this comment

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

Changes reviewed

GitHubAssignees.ps1 Outdated Show resolved Hide resolved
@giuseppecampanelli
Copy link
Contributor Author

giuseppecampanelli commented May 29, 2020

Removed them 😄

@HowardWolosky
Copy link
Member

It looks like some conflicts came in as a result of some of the recent commits as I've been trying to get the CI to run clean on all platforms. Can you please address the conflicts? Then I'll give it a run through the CI and get it committed....Thanks!

@giuseppecampanelli
Copy link
Contributor Author

Resolved conflicts 😄

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

This is ready to go. Thanks so much!

@HowardWolosky HowardWolosky merged commit a6a27aa into microsoft:master Jun 1, 2020
@HowardWolosky HowardWolosky added the breaking change This includes a change in existing published module behavior. label Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This includes a change in existing published module behavior. technical debt Work that was postponed by a previous change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove- commands should prompt for confirmation
3 participants