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

Fix on_denied and on_missing bugs #618

Merged
merged 5 commits into from
Mar 28, 2022

Conversation

jsf9k
Copy link
Contributor

@jsf9k jsf9k commented Jan 14, 2022

SUMMARY

This pull request:

  • Changes the default value of on_denied to be error, so that it agrees with what is stated in the documentation.
  • Changes the default value of on_missing to be error, and updates the documentation to explain this.

Fixes #617.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

aws_ssm lookup

@jsf9k jsf9k changed the title Fix on_denied and on_missing documentation bugs Fix on_denied and on_missing bugs Jan 14, 2022
@softwarefactory-project-zuul
Copy link
Contributor

@abikouo
Copy link
Contributor

abikouo commented Jan 17, 2022

@jsf9k thanks for taking the time to submit this pull request.
Isn't this leading to a breaking change?

@goneri
Copy link
Member

goneri commented Jan 17, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

@jsf9k
Copy link
Contributor Author

jsf9k commented Jan 18, 2022

The Zuul build failures appear unrelated to the changes I made.

@jsf9k
Copy link
Contributor Author

jsf9k commented Jan 18, 2022

@jsf9k thanks for taking the time to submit this pull request. Isn't this leading to a breaking change?

@abikouo - Yes and no, since the documentation is self-contradictory. The changes I made agree with the documentation here and here, although they disagree with the documentation here.

@jsf9k
Copy link
Contributor Author

jsf9k commented Jan 21, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

@alinabuzachis
Copy link
Collaborator

alinabuzachis commented Jan 25, 2022

@jsf9k Thank your for taking time to work on this. I was just wondering if the mismatch is only here https://github.com/ansible-collections/amazon.aws/pull/618/files#diff-eee5beb719b6b892c1ebd6a98f7120ad8ab12ccbafaa9b6604938f08da88b7a2L177 and on_denied should default to error. I don't have so much experience with lookup plugins, but looking at aws_secret lookup plugin, it seems both parameters (on_denied and on_missing) default to error. I'll wait for confirmation from @jillr or @tremble.

@alinabuzachis
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link
Contributor

@jsf9k
Copy link
Contributor Author

jsf9k commented Jan 25, 2022

@jsf9k Thank your for taking time to work on this. I was just wondering if the mismatch is only here https://github.com/ansible-collections/amazon.aws/pull/618/files#diff-eee5beb719b6b892c1ebd6a98f7120ad8ab12ccbafaa9b6604938f08da88b7a2L177 and on_denied should default to error. I don't have so much experience with lookup plugins, but looking at aws_secret lookup plugin, it seems both parameters (on_denied and on_missing) default to error. I'll wait for confirmation from @jillr or @tremble.

I'm fine with that @alinabuzachis. Just give me the word and I can do a rebase to this PR to make those changes instead. Or I'll wait for direction from @tremble or @jillr.

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review lookup lookup plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Jan 27, 2022
@jsf9k
Copy link
Contributor Author

jsf9k commented Feb 3, 2022

recheck

@jsf9k
Copy link
Contributor Author

jsf9k commented Feb 3, 2022

@jsf9k Thank your for taking time to work on this. I was just wondering if the mismatch is only here https://github.com/ansible-collections/amazon.aws/pull/618/files#diff-eee5beb719b6b892c1ebd6a98f7120ad8ab12ccbafaa9b6604938f08da88b7a2L177 and on_denied should default to error. I don't have so much experience with lookup plugins, but looking at aws_secret lookup plugin, it seems both parameters (on_denied and on_missing) default to error. I'll wait for confirmation from @jillr or @tremble.

@alinabuzachis - If what you stated is true, we would still need to modify the documentation here. The statement there pretty clearly indicates that the default for on_missing is skip.

I agree 100% that the behavior of this lookup plugin and that of aws_secret should be consistent. Should I go ahead and make those changes here, or are we still waiting for word from @tremble or @jillr?

@softwarefactory-project-zuul
Copy link
Contributor

@jillr jillr removed the needs_triage label Feb 8, 2022
@jillr
Copy link
Contributor

jillr commented Feb 9, 2022

aws_secret and the base Lookup class both default to error, so it feels like the best thing to do is keep the behaviour generally consistent between lookup plugins that implement these options. It is breaking, but in a bugfix way, so as long as we include a changelog fragment with the right category and don't backport it to 3.x I think setting it to error is ok.
Sorry for the delay getting back to you @jsf9k!

@softwarefactory-project-zuul
Copy link
Contributor

@goneri
Copy link
Member

goneri commented Mar 23, 2022

@alinabuzachis well the CI running as expected. Github is again facing some problems (https://www.githubstatus.com/), this may explain why it was a bit slow.

@alinabuzachis
Copy link
Collaborator

@alinabuzachis well the CI running as expected. Github is again facing some problems (https://www.githubstatus.com/), this may explain why it was a bit slow.

Thanks!

@alinabuzachis
Copy link
Collaborator

recheck

1 similar comment
@alinabuzachis
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@jsf9k
Copy link
Contributor Author

jsf9k commented Mar 25, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

@alinabuzachis
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link
Contributor

@jsf9k
Copy link
Contributor Author

jsf9k commented Mar 25, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

@alinabuzachis
Copy link
Collaborator

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@jsf9k
Copy link
Contributor Author

jsf9k commented Mar 26, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@alinabuzachis alinabuzachis added the mergeit Merge the PR (SoftwareFactory) label Mar 28, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build failed (gate pipeline). For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@alinabuzachis alinabuzachis added mergeit Merge the PR (SoftwareFactory) and removed mergeit Merge the PR (SoftwareFactory) labels Mar 28, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit e85e420 into ansible-collections:main Mar 28, 2022
@patchback
Copy link

patchback bot commented Mar 28, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/e85e42094765b9d7df85f18b01a70d2ff44ce161/pr-618

Backported as #747

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Mar 28, 2022
Fix on_denied and on_missing bugs

SUMMARY
This pull request:

Changes the default value of on_denied to be error, so that it agrees with what is stated in the documentation.
Changes the default value of on_missing to be error, and updates the documentation to explain this.

Fixes #617.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
aws_ssm lookup

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Shane Frasier <[email protected]>
Reviewed-by: Markus Bergholz <[email protected]>
(cherry picked from commit e85e420)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Mar 28, 2022
[PR #618/e85e4209 backport][stable-3] Fix on_denied and on_missing bugs

This is a backport of PR #618 as merged into main (e85e420).
SUMMARY
This pull request:

Changes the default value of on_denied to be error, so that it agrees with what is stated in the documentation.
Changes the default value of on_missing to be error, and updates the documentation to explain this.

Fixes #617.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
aws_ssm lookup
@markuman markuman mentioned this pull request Mar 31, 2022
alinabuzachis added a commit to alinabuzachis/amazon.aws that referenced this pull request Mar 31, 2022
alinabuzachis added a commit to alinabuzachis/amazon.aws that referenced this pull request Mar 31, 2022
alinabuzachis added a commit to alinabuzachis/amazon.aws that referenced this pull request Mar 31, 2022
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Apr 5, 2022
Revert "Fix on_denied and on_missing bugs (#618) (#747)"

SUMMARY

This reverts commit 83a9db6 because it introduces a breaking change that does not need to be included in the current release.

ISSUE TYPE


Bugfix Pull Request
Docs Pull Request
Feature Pull Request
New Module Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Jill R <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3 PR should be backported to the stable-3 branch bug This issue/PR relates to a bug community_review lookup lookup plugin mergeit Merge the PR (SoftwareFactory) new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_ssm lookup documentation disagrees with code
7 participants