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 custom file attributes for public keys #257

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

crwr45
Copy link
Contributor

@crwr45 crwr45 commented Jul 20, 2021

Use of the confusingly-named _permissions_changed() on both
sides of an or was resulting in the second invocation not
being reached if the first invocation returned True.

This is fixed by explicitly running both invocations
before evaluating the return values.

Signed-off-by: Charlie Wheeler-Robinson cwheeler@redhat.com

ISSUE TYPE
  • Bugfix Pull Request

@felixfontein
Copy link
Contributor

Thanks for your contributoin! Can you please add a changelog fragment?

Use of the confusingly-named _permissions_changed() on both
sides of an `or` was resulting in the second invocation not
being reached if the first invocation returned True, which it
does any time it applied custom attributes to the private key.
As a result, custom file attributes were only ever being
applied to the private key (except in one specific case)

This is fixed by explicitly updating attributes of both files
before checking if changes have been made.

Signed-off-by: Charlie Wheeler-Robinson <cwheeler@redhat.com>
Copy link
Collaborator

@Ajpantuso Ajpantuso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!
The generation and validation logic in this module is still pending re-write and one of the items is to make _permissions_changed into a command like update_permissions which updates self.changed directly.

@felixfontein felixfontein merged commit 6c989de into ansible-collections:main Jul 20, 2021
@felixfontein
Copy link
Contributor

@crwr45 thanks for fixing this!
@Ajpantuso thanks for reviewing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants