ansible-powerscale icon indicating copy to clipboard operation
ansible-powerscale copied to clipboard

Add s3_key module

Open fpfuetsch opened this issue 1 month ago β€’ 12 comments

Description

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

GitHub Issues

Fixes #207

Checklist:

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

How Has This Been Tested?

  • [X] manual testing by building the collection locally
  • [x] automated tests

fpfuetsch avatar Oct 02 '25 06:10 fpfuetsch

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 avatar Oct 02 '25 06:10 fpfuetsch

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 avatar Oct 03 '25 01:10 anupamaloke

@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)

anupamaloke avatar Oct 03 '25 01:10 anupamaloke

Codecov Report

:x: Patch coverage is 92.30769% with 19 lines in your changes missing coverage. Please review. :white_check_mark: 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 :warning:
tests/unit/plugins/module_utils/mock_s3_key_api.py 95.23% 0 Missing and 1 partial :warning:
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%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov-commenter avatar Oct 03 '25 02:10 codecov-commenter

@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. πŸ€”

fpfuetsch avatar Oct 06 '25 12:10 fpfuetsch

@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.

anupamaloke avatar Oct 06 '25 18:10 anupamaloke

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

fpfuetsch avatar Oct 09 '25 11:10 fpfuetsch

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

fpfuetsch avatar Oct 14 '25 10:10 fpfuetsch

@anupamaloke hopefully fixed now all sanity and linting errors

fpfuetsch avatar Oct 15 '25 06:10 fpfuetsch

@anupamaloke What are the next steps now? :)

fpfuetsch avatar Oct 21 '25 07:10 fpfuetsch

@anupamaloke @ShrinidhiRao15 @gokul-srivathsan anybody there? πŸ˜Άβ€πŸŒ«οΈ

fpfuetsch avatar Oct 31 '25 09:10 fpfuetsch

@fpfuetsch , will check this.

gokul-srivathsan avatar Nov 03 '25 00:11 gokul-srivathsan