Skip to content

Conversation

@fpfuetsch
Copy link

@fpfuetsch fpfuetsch commented Oct 2, 2025

Description

This PR adds the module s3_key which allows the management of S3 keys for users.

GitHub Issues

Fixes #207

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, pep8, linting, or security issues
  • I have performed Ansible Sanity test using --docker default
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Backward compatibility is not broken

How Has This Been Tested?

  • manual testing by building the collection locally
  • automated tests

@fpfuetsch
Copy link
Author

HELP: I would like to add automated tests but did not manage to execute the existing tests locally. Is there any documentation I missed describing how the test should be executed locally?

@fpfuetsch fpfuetsch changed the title Add s3_key plugin module Add s3_key module Oct 2, 2025
@anupamaloke
Copy link
Collaborator

HELP: I would like to add automated tests but did not manage to execute the existing tests locally. Is there any documentation I missed describing how the test should be executed locally?

@fpfuetsch, please see Testing Ansible and Collections for details on how to go about running the ansible sanity, unit and integration test cases. Also, see Testing collections.

@anupamaloke
Copy link
Collaborator

@fpfuetsch, you might want to fix the below module documentation error which is leading to ansible-sanity failing:

ERROR! module dellemc.powerscale.s3_key missing documentation (or could not parse documentation): dellemc.powerscale.s3_key did not contain a DOCUMENTATION attribute (/home/runner/work/ansible-powerscale/ansible-powerscale/.tox/sanity-py3.10-2.17/tmp/collections/ansible_collections/dellemc/powerscale/plugins/modules/s3_key.py). Unable to parse documentation in python file '/home/runner/work/ansible-powerscale/ansible-powerscale/.tox/sanity-py3.10-2.17/tmp/collections/ansible_collections/dellemc/powerscale/plugins/modules/s3_key.py': f-string: unmatched '(' (<unknown>, line 304). f-string: unmatched '(' (<unknown>, line 304)

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.38%. Comparing base (da078f2) to head (d9acda7).

Files with missing lines Patch % Lines
plugins/modules/s3_key.py 85.12% 10 Missing and 8 partials ⚠️
tests/unit/plugins/module_utils/mock_s3_key_api.py 95.23% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
+ Coverage   90.35%   90.38%   +0.02%     
==========================================
  Files         143      146       +3     
  Lines       16766    17013     +247     
  Branches     2317     2336      +19     
==========================================
+ Hits        15149    15377     +228     
- Misses        946      956      +10     
- Partials      671      680       +9     
Flag Coverage Δ
units 90.38% <92.30%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@anupamaloke anupamaloke left a comment

Choose a reason for hiding this comment

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

Please fix the pylint errors.

@fpfuetsch
Copy link
Author

@anupamaloke Okay will have a look.

What do you think about the argument existing_key_overwrite: true/false to trigger the replacement of existing S3 keys. The thing I do not like about it that it violates idempotency. An alternative I could think about is to provide some kind of timestamp which is compared with the existing key's creation date to decide if a new key should be created. 🤔

@anupamaloke
Copy link
Collaborator

anupamaloke commented Oct 6, 2025

@anupamaloke Okay will have a look.

Thank you! It seems there are a few more in the latest sanity run and if you could fix them as well?

ERROR: Found 4 yamllint issue(s) which need to be resolved:
ERROR: plugins/modules/s3_key.py:33:9: error: DOCUMENTATION: syntax error: could not find expected ':' (syntax)
ERROR: plugins/modules/s3_key.py:33:9: unparsable-with-libyaml: DOCUMENTATION: while scanning a simple key - could not find expected ':'
ERROR: plugins/modules/s3_key.py:115:24: error: RETURN: syntax error: could not find expected ':' (syntax)
ERROR: plugins/modules/s3_key.py:115:24: unparsable-with-libyaml: RETURN: while scanning a simple key - could not find expected ':'

What do you think about the argument existing_key_overwrite: true/false to trigger the replacement of existing S3 keys. The thing I do not like about it that it violates idempotency. An alternative I could think about is to provide some kind of timestamp which is compared with the existing key's creation date to decide if a new key should be created. 🤔

Yeah, this is the challenge with idempotency for all the modules that we have for configuring passwods/secrets/creds. One way I think of handling it would be like how it has been done in the ansible.builtin.user module with the update_password argument.

On the similar lines, we can introduce a new argument called update_s3_key (may be rename the existing_key_overwrite argument). Then, users will be able to use the argument choices - update_s3_key: always or update_s3_key: oncreate - to control the key update workflow as well as the idempotency. Let me know what you think.

@fpfuetsch
Copy link
Author

@anupamaloke Thanks for your input. Tests were added and existing_key_overwrite was changed to generate_new_key: 'always'/'if_not_present'

@fpfuetsch
Copy link
Author

@anupamaloke it would be nice if you could approve the workflow run for the latest commit :)

@fpfuetsch
Copy link
Author

fpfuetsch commented Oct 15, 2025

@anupamaloke hopefully fixed now all sanity and linting errors

@fpfuetsch fpfuetsch force-pushed the feature-207-s3key-support branch 5 times, most recently from f8984b5 to 167f2ef Compare October 16, 2025 06:55
Signed-off-by: fpfuetsch <54020707+fpfuetsch@users.noreply.github.com>
Signed-off-by: fpfuetsch <54020707+fpfuetsch@users.noreply.github.com>
Signed-off-by: fpfuetsch <54020707+fpfuetsch@users.noreply.github.com>
Signed-off-by: fpfuetsch <54020707+fpfuetsch@users.noreply.github.com>
Signed-off-by: fpfuetsch <54020707+fpfuetsch@users.noreply.github.com>
@fpfuetsch fpfuetsch force-pushed the feature-207-s3key-support branch from 167f2ef to a8db076 Compare October 16, 2025 07:08
anupamaloke
anupamaloke previously approved these changes Oct 16, 2025
Copy link
Collaborator

@anupamaloke anupamaloke left a comment

Choose a reason for hiding this comment

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

LGTM

@fpfuetsch
Copy link
Author

@anupamaloke What are the next steps now? :)

@fpfuetsch
Copy link
Author

@anupamaloke @ShrinidhiRao15 @gokul-srivathsan anybody there? 😶‍🌫️

@gokul-srivathsan
Copy link

@fpfuetsch , will check this.

@fpfuetsch
Copy link
Author

@anupamaloke @gokul-srivathsan
What's the deal with this repository? #215 was merged within 2 days. My PR is now open for 2 months (among 2 others) without any progress from the reviewers side. Are external contributions not welcome? 😞

@Zaubeamit
Copy link

My name is Amit, and I’m the new Chief Product Owner for the Ansible team. We will prioritize this PR with our new Product Manager. Thank you for your patience.
I will do the same for the other 2 PRs. Thanks again

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.

[FEATURE]: Add support for S3 Key management

6 participants