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

Allow side_effect and verify to have parameters #3730

Merged
merged 2 commits into from
Dec 5, 2022
Merged

Allow side_effect and verify to have parameters #3730

merged 2 commits into from
Dec 5, 2022

Conversation

ehartmann
Copy link
Contributor

Fix #3729

@ehartmann ehartmann requested review from a team as code owners November 8, 2022 14:10
@ehartmann ehartmann requested review from ziegenberg, odyssey4me, trishnaguha, cidrblock and priyamsahoo and removed request for a team November 8, 2022 14:10
@ehartmann ehartmann force-pushed the bug/eh/molecule_schema_for_side_effect branch 2 times, most recently from 945f318 to 2be6533 Compare November 8, 2022 14:27
@ehartmann ehartmann changed the title Allow side_effect to have parameters Allow side_effect and verify to have parameters Nov 8, 2022
@ehartmann ehartmann force-pushed the bug/eh/molecule_schema_for_side_effect branch from fac8e80 to 0e4bef5 Compare November 8, 2022 16:31
@ssbarnea ssbarnea added the bug label Nov 9, 2022
ssbarnea
ssbarnea requested changes Nov 9, 2022
View reviewed changes
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

Do not remove enums, they are key for providing IDE integration. To allow other custom values, allow both enum and a free form string, keeping enum as the first option. We already use this in other places.

@ehartmann
Copy link
Contributor Author

Hey @ssbarnea , thanks for taking the time to have a look at this PR.

I've just pushed another way of allowing side_effect and verify to take arguments as you suggested. Let me know if you need anything else.

@zhan9san zhan9san force-pushed the bug/eh/molecule_schema_for_side_effect branch from 93e997b to dc97cca Compare November 15, 2022 05:30
@zhan9san
Copy link
Contributor

@ssbarnea
Do you have any concern about this PR?

The PR cannot be merged until you approve it.

@zhan9san zhan9san force-pushed the bug/eh/molecule_schema_for_side_effect branch from dc97cca to 6820468 Compare November 15, 2022 07:12
@ssbarnea ssbarnea self-requested a review December 5, 2022 14:56
@ssbarnea ssbarnea merged commit e39341e into ansible:main Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhan9san zhan9san zhan9san approved these changes

@ssbarnea ssbarnea ssbarnea approved these changes

@ziegenberg ziegenberg Awaiting requested review from ziegenberg ziegenberg was automatically assigned from ansible-community/molecule

@odyssey4me odyssey4me Awaiting requested review from odyssey4me odyssey4me was automatically assigned from ansible-community/molecule

@trishnaguha trishnaguha Awaiting requested review from trishnaguha trishnaguha was automatically assigned from ansible-community/devtools

@cidrblock cidrblock Awaiting requested review from cidrblock cidrblock was automatically assigned from ansible-community/devtools

@priyamsahoo priyamsahoo Awaiting requested review from priyamsahoo priyamsahoo was automatically assigned from ansible-community/devtools

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Schema validation does not allow to specify side_effects and verify script
3 participants
@ehartmann @zhan9san @ssbarnea