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

vdo: add force option #2110

Merged
merged 6 commits into from
Mar 27, 2021
Merged

vdo: add force option #2110

merged 6 commits into from
Mar 27, 2021

Conversation

aminvakil
Copy link
Contributor

SUMMARY

Fixes #2101

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

vdo

@ansibullbot
Copy link
Collaborator

cc @bgurney-rh @rhawalsh
click here for bot help

@ansibullbot ansibullbot added affects_2.10 community_review feature This issue/PR relates to a feature request module module needs_triage plugins plugin (any type) system labels Mar 25, 2021
aminvakil
aminvakil commented Mar 26, 2021
View reviewed changes
plugins/modules/system/vdo.py
@@ -258,6 +258,15 @@
configured setting unless a different value is specified
in the playbook.
type: str
force:
description:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken the description from man page in
https://github.com/dm-vdo/vdo/blob/827e5e0974c62a4e169766020fb69b3f34015851/vdo-manager/man/vdo.8#L580-L584
Please change it in any way you see fit.

@felixfontein
Copy link
Collaborator

@rhawalsh @Davidffry can you please take a look at this PR? Does it implements what you want, and is the description and the warning good enough from your point of view?

rhawalsh
rhawalsh approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@rhawalsh rhawalsh left a comment

Choose a reason for hiding this comment

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

Hi @felixfontein
I took a look at it a little while ago. The only feedback I had was around suggesting some warning text. I think what you have should do the trick. I think the change itself overall, looks good to me.

plugins/modules/system/vdo.py Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

@rhawalsh it looks like you forgot to submit the review, because your suggestion for the warning text only showed up now :)
@aminvakil I unresolved the comment in question, PTAL at @rhawalsh's suggestion.

@felixfontein felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Mar 27, 2021
rhawalsh
rhawalsh approved these changes Mar 27, 2021
View reviewed changes
Copy link
Contributor

@rhawalsh rhawalsh left a comment

Choose a reason for hiding this comment

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

Hi @felixfontein , thanks for adjusting. I just figured that you didn't agree. :) I appreciate the work you've done on this. The change looks good to me.

@felixfontein felixfontein merged commit 73bb0f1 into ansible-collections:main Mar 27, 2021
patchback bot pushed a commit that referenced this pull request Mar 27, 2021
vdo: add force option (#2110)
86edf87 
* vdo: add force option

* Add changelog

* Improve the diff the next time something is added :)

Co-authored-by: Felix Fontein <[email protected]>

* Add warning text placeholder by felixfontein

Co-authored-by: Felix Fontein <[email protected]>

* Add warning text

* Apply suggestion for warning text from rhawalsh

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 73bb0f1)
@patchback patchback bot mentioned this pull request Mar 27, 2021
Merged
@felixfontein
Copy link
Collaborator

@aminvakil thanks for implementing this!
@rhawalsh thanks for reviewing!

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Mar 27, 2021
felixfontein pushed a commit that referenced this pull request Mar 27, 2021
@patchback
vdo: add force option (#2110) (#2123)
0d28bfb 
* vdo: add force option

* Add changelog

* Improve the diff the next time something is added :)

Co-authored-by: Felix Fontein <[email protected]>

* Add warning text placeholder by felixfontein

Co-authored-by: Felix Fontein <[email protected]>

* Add warning text

* Apply suggestion for warning text from rhawalsh

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 73bb0f1)

Co-authored-by: Amin Vakil <[email protected]>
@aminvakil
Copy link
Contributor Author

@felixfontein @rhawalsh Thanks for reviewing!

@aminvakil aminvakil deleted the vdo_force branch March 27, 2021 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhawalsh rhawalsh rhawalsh approved these changes

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
community_review feature This issue/PR relates to a feature request module module plugins plugin (any type) system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add "force" option for vdo modules
4 participants
@aminvakil @ansibullbot @felixfontein @rhawalsh