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

Support min max object size in s3_lifecycle module #2205

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leo4ever
Copy link

SUMMARY

Support the S3 lifecycle settings of minimum and maximum object size to apply the lifecycle rules.

Fixes #861

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

s3_lifecycle

ADDITIONAL INFORMATION

Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/f08060c4c200444dafdd5919d421be14

✔️ ansible-galaxy-importer SUCCESS in 4m 13s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 28s
✔️ ansible-test-splitter SUCCESS in 4m 56s
✔️ integration-community.aws-1 SUCCESS in 32m 10s
Skipped 21 jobs

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to submit this, especially for including the new integration tests.

If you take a look at the output from the tests you'll see a couple of issues:

These boil down to

  • The typo on line 63 breaks the documentation (YAML)
  • We use the "black" formatting style, If you've got tox setup, then tox -m format it should fix the formatting.

WRT Unit tests:
Unit tests can be a PITA to implement (they live under tests/unit/plugins/modules/).
I'd really like to see some basic tests for filters_are_equal if possible. However, by moving filters_are_equal outside of compare_and_update_configuration you'll make it easier for someone (possibly me) to come along later and add them if you're not able to do so.

@@ -59,6 +59,16 @@
and replaced with the new transition(s)
default: true
type: bool
maximum_object_size:
descriptionL
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
descriptionL
description:

Copy link
Author

Choose a reason for hiding this comment

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

@tremble - I have made the changes based on your comments. But still seeing the build on collections docs failing - https://github.com/ansible-collections/community.aws/actions/runs/12403295884

I have created a unit test but having trouble trying to run tests locally. Can this test be executed as part of CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

@leo4ever

If you add it to tests/unit/plugins/modules/test_s3_lifecycle.py it'll be run automatically by CI. In some cases we need to manually approve running the tests which is why you'll sometimes see a delay.

I wanted to respond a couple of days ago and tell you it should be possible to run the tests via tox, only to realise that an updated python library somewhere caused some breakage. (#2206 should fix this)

Normally it should be possible to trigger the unit tests with
tox -m unit -- tests/unit/plugins/modules/
By default this triggers tests with a variety of Ansible and Python versions.

plugins/modules/s3_lifecycle.py Outdated Show resolved Hide resolved
plugins/modules/s3_lifecycle.py Outdated Show resolved Hide resolved
plugins/modules/s3_lifecycle.py Outdated Show resolved Hide resolved
2. Updated code based on the feedback from PR review
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/f9dfd0268ea947d3a2ec58131bd81d4f

ansible-galaxy-importer FAILURE in 5m 44s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 32s
✔️ ansible-test-splitter SUCCESS in 3m 56s
✔️ integration-community.aws-1 SUCCESS in 26m 13s
Skipped 21 jobs

Comment on lines +632 to +633
maximum_object_size=dict(type=int),
minimum_object_size=dict(type=int),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
maximum_object_size=dict(type=int),
minimum_object_size=dict(type=int),
maximum_object_size=dict(type="int"),
minimum_object_size=dict(type="int"),

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.

s3_lifecycle - Add size thresholds
2 participants