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 no_log=False to clear false-positives #156

Conversation

quidame
Copy link
Contributor

@quidame quidame commented Mar 16, 2021

SUMMARY

Ansible sanity test validate-modules triggers false-positives and hints no-log-needed errors for module parameters containing either key or passin their names, as in:

2021-03-16 20:27:04.127992 | centos-8 | ERROR: Found 3 validate-modules issue(s) which need to be resolved:
2021-03-16 20:27:04.128105 | centos-8 | ERROR: plugins/modules/authorized_key.py:0:0: no-log-needed: Argument 'key' in argument_spec could be a secret, though doesn't have `no_log` set
2021-03-16 20:27:04.128204 | centos-8 | ERROR: plugins/modules/authorized_key.py:0:0: no-log-needed: Argument 'key_options' in argument_spec could be a secret, though doesn't have `no_log` set
2021-03-16 20:27:04.128303 | centos-8 | ERROR: plugins/modules/mount.py:0:0: no-log-needed: Argument 'passno' in argument_spec could be a secret, though doesn't have `no_log` set

This PR bypasses these errors by explicitly setting no_log=False in module's argument_spec.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

mount
authorized_key

ADDITIONAL INFORMATION

See also ansible-collections/community.network@af60326

@Akasurde
Copy link
Member

recheck

Related modules: authorized_key, mount.
@quidame quidame force-pushed the fix/no-log-needed_false_positive branch from 403c581 to c8b5887 Compare March 19, 2021 08:09
@quidame
Copy link
Contributor Author

quidame commented Mar 19, 2021

It seems Ansible Zuul check ansible-changelog-fragment is a little bit tricky. https://github.com/ansible/project-config/blob/master/playbooks/ansible-changelog-fragment/run.yaml says:

shell: git show --name-status --exit-code --oneline --diff-filter=A | grep changelogs/fragments/

This succeeds if, and only if, a changelog fragment has been added (not modified) in the last commit (not the previous ones). That means that the last commit MUST contain the add of the changelog fragment.

I squashed all commits of the PR and force-pushed to make the changelog fragment added in the last commit, and ansible-changelog-fragment succeeded this time.

Is it the expected behavior ?

@Andersson007
Copy link
Contributor

@justjais ^^

@pabelanger
Copy link
Contributor

Yes, each PR needs a changelog fragment. However, the way that github PRs work, you can push up a new commit in your PR (with that changelog) and the check system will pass.

Or you can squash as you did for a single commit.

@quidame
Copy link
Contributor Author

quidame commented Mar 20, 2021

@pabelanger why not, but the error message says:

"Your pull-request is missing a changelog fragment, please add one. It should explain to end users the reason for your change."

I'm just clever enough to understand that something else than the changelog fragment is missing, because I know there is a changelog fragment. The message didn't help me. I find it confusing when, say, a contributor applies changes suggested by a reviewer, and then what was a successful check suddenly fails over and over. Only the read of the source code and testing my first hypothesis after reading the git-show's manual page lead me to a solution to make the check succeeds.

If the message says the PR is missing a changelog fragment and the PR contains a changelog fragment, my obvious conclusion is that the message is wrong. And if the current behavior is the expected one, as you explained me, I think the message is definitively wrong.

"Your last commit is missing a changelog fragment. It should explain to end users the reason for your change. Please add one, or if already present in a previous commit, squash them to make the changelog fragment added in the last commit."

or something else would be of better help :) thanks

@Andersson007
Copy link
Contributor

cc @Akasurde

@Akasurde Akasurde added the gate Gate PR in Zuul CI (Obsolete: Please set "mergeit" instead of "gate") label Mar 24, 2021
@ansible-zuul ansible-zuul bot merged commit 355a99f into ansible-collections:main Mar 24, 2021
@Akasurde
Copy link
Member

@quidame Thanks for the contribution. @Andersson007 Thanks for the review.

@Andersson007
Copy link
Contributor

@quidame @Akasurde @pabelanger thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gate Gate PR in Zuul CI (Obsolete: Please set "mergeit" instead of "gate")
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants