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

s3_lifecycle: support value '0' for transition_days #1077

Conversation

fmenabe
Copy link
Contributor

@fmenabe fmenabe commented Apr 19, 2022

SUMMARY

s3_lifecycle module does not support value 0 for transition_days parameter.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

s3_lifecycle

ADDITIONAL INFORMATION

A lifecycle rule with 0 as transition_days allows to create a rule that will move objects with a delta of just few hours (if set to 1, objects will be moved with a delta of 1d + few hours).

When the value 0 is set, the value is stripped from the query and we get an error that is hard to correlate with an "invalid" value on this parameter (which is valid as 0 is an integer):

- name: Create s3 buckets lifecycle rules
  s3_lifecycle:
    ec2_url: "FIXME"
    region: "FIXME"
    aws_access_key: "FIXME"
    aws_secret_key: "FIXME"
    name: "FIXME"
    rule_id: "onezone"
    transitions:
    - transition_days: 0
      storage_class: "ONEZONE_IA"
    state: present
    status: enabled

fatal: [localhost]: FAILED! => {
    "boto3_version": "1.21.3",
    "botocore_version": "1.24.19",
    "changed": false,
    "error": {
        "code": "MalformedXML",
        "message": "Extra element Transition in interleave"
    },
    "lifecycle_configuration": {
        "Rules": [
            {
                "Filter": {
                    "Prefix": ""
                },
                "ID": "onezone",
                "Status": "Enabled",
                "Transitions": [
                    {
                        "StorageClass": "ONEZONE_IA"
                    }
                ]
            }
        ]
    },
    ...
MSG:

An error occurred (MalformedXML) when calling the PutBucketLifecycleConfiguration operation: Extra element Transition in interleave

This is because transition.get('transition_days') returns 0 which is considered as False on a condition (this patch just check if the value is defined; e.g. is not None).

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Apr 19, 2022
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 raise this PR.

The change looks good at first glance. If possible please would you:

  1. Add integration tests to tests/integration/targets/s3_lifecycle/tasks/main.yml
  2. Add a changelog entry

@fmenabe fmenabe force-pushed the s3_lifecycle_transition_days branch from 8f0f3cb to fdf2296 Compare April 19, 2022 22:33
@ansibullbot ansibullbot added integration tests/integration tests tests and removed small_patch Hopefully easy to review labels Apr 19, 2022
@fmenabe fmenabe force-pushed the s3_lifecycle_transition_days branch from fdf2296 to 5568769 Compare April 19, 2022 23:25
@markuman markuman added backport-2 PR should be backported to the stable-2 branch backport-3 PR should be backported to the stable-3 branch labels Apr 20, 2022
@fmenabe fmenabe force-pushed the s3_lifecycle_transition_days branch from 5568769 to 583f077 Compare April 20, 2022 07:43
@fmenabe
Copy link
Contributor Author

fmenabe commented Apr 20, 2022

Tests still fail because standard_ia and onezone_ia expect a transition days greater than 30 days (server side). I was going to force push my PR another time to move the tests on glacier storage class but I see you approved the changes, so should I do it?

@markuman
Copy link
Member

Tests still fail because standard_ia and onezone_ia expect a transition days greater than 30 days (server side). I was going to force push my PR another time to move the tests on glacier storage class but I see you approved the changes, so should I do it?

Simply move on in this PR :) it won't be merged as long as the ci is failing.

@fmenabe fmenabe force-pushed the s3_lifecycle_transition_days branch from 583f077 to e725b3c Compare April 20, 2022 08:16
@tremble
Copy link
Contributor

tremble commented Apr 20, 2022

but I see you approved the changes, so should I do it?

The approval was to recognise that I'm happy with the code change and the changelog fragment. Broken tests will need to be fixed.

@markuman markuman added the mergeit Merge the PR (SoftwareFactory) label Apr 20, 2022
@markuman
Copy link
Member

@fmenabe thank you for your contribution.

@fmenabe
Copy link
Contributor Author

fmenabe commented Apr 20, 2022

@tremble @markuman thanks for your quick review and help!

No hurry for me but by curiosity, how much time does it take generally to go upstream?

@markuman
Copy link
Member

@tremble @markuman thanks for your quick review and help!

No hurry for me but by curiosity, how much time does it take generally to go upstream?

there are no release dates yet: #890
I guess something between 3 and 6 weeks.

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 4f989bd into ansible-collections:main Apr 20, 2022
@patchback
Copy link

patchback bot commented Apr 20, 2022

Backport to stable-2: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-2/4f989bdd5578373f97ce31008f50a15c825af5ef/pr-1077

Backported as #1082

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Apr 20, 2022
s3_lifecycle: support value '0' for transition_days

SUMMARY
s3_lifecycle module does not support value 0 for transition_days parameter.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
s3_lifecycle
ADDITIONAL INFORMATION
A lifecycle rule with 0 as transition_days allows to create a rule that will move objects with a delta of just few hours (if set to 1, objects will be moved with a delta of 1d + few hours).
When the value 0 is set, the value is stripped from the query and we get an error that is hard to correlate with an "invalid"  value on this parameter (which is valid as 0 is an integer):
- name: Create s3 buckets lifecycle rules
  s3_lifecycle:
    ec2_url: "FIXME"
    region: "FIXME"
    aws_access_key: "FIXME"
    aws_secret_key: "FIXME"
    name: "FIXME"
    rule_id: "onezone"
    transitions:
    - transition_days: 0
      storage_class: "ONEZONE_IA"
    state: present
    status: enabled

fatal: [localhost]: FAILED! => {
    "boto3_version": "1.21.3",
    "botocore_version": "1.24.19",
    "changed": false,
    "error": {
        "code": "MalformedXML",
        "message": "Extra element Transition in interleave"
    },
    "lifecycle_configuration": {
        "Rules": [
            {
                "Filter": {
                    "Prefix": ""
                },
                "ID": "onezone",
                "Status": "Enabled",
                "Transitions": [
                    {
                        "StorageClass": "ONEZONE_IA"
                    }
                ]
            }
        ]
    },
    ...
MSG:

An error occurred (MalformedXML) when calling the PutBucketLifecycleConfiguration operation: Extra element Transition in interleave

This is because transition.get('transition_days') returns 0 which is considered as False on a condition (this patch just check if the value is defined; e.g. is not None).

Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <git@osuv.de>
(cherry picked from commit 4f989bd)
@patchback
Copy link

patchback bot commented Apr 20, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/4f989bdd5578373f97ce31008f50a15c825af5ef/pr-1077

Backported as #1083

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Apr 20, 2022
s3_lifecycle: support value '0' for transition_days

SUMMARY
s3_lifecycle module does not support value 0 for transition_days parameter.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
s3_lifecycle
ADDITIONAL INFORMATION
A lifecycle rule with 0 as transition_days allows to create a rule that will move objects with a delta of just few hours (if set to 1, objects will be moved with a delta of 1d + few hours).
When the value 0 is set, the value is stripped from the query and we get an error that is hard to correlate with an "invalid"  value on this parameter (which is valid as 0 is an integer):
- name: Create s3 buckets lifecycle rules
  s3_lifecycle:
    ec2_url: "FIXME"
    region: "FIXME"
    aws_access_key: "FIXME"
    aws_secret_key: "FIXME"
    name: "FIXME"
    rule_id: "onezone"
    transitions:
    - transition_days: 0
      storage_class: "ONEZONE_IA"
    state: present
    status: enabled

fatal: [localhost]: FAILED! => {
    "boto3_version": "1.21.3",
    "botocore_version": "1.24.19",
    "changed": false,
    "error": {
        "code": "MalformedXML",
        "message": "Extra element Transition in interleave"
    },
    "lifecycle_configuration": {
        "Rules": [
            {
                "Filter": {
                    "Prefix": ""
                },
                "ID": "onezone",
                "Status": "Enabled",
                "Transitions": [
                    {
                        "StorageClass": "ONEZONE_IA"
                    }
                ]
            }
        ]
    },
    ...
MSG:

An error occurred (MalformedXML) when calling the PutBucketLifecycleConfiguration operation: Extra element Transition in interleave

This is because transition.get('transition_days') returns 0 which is considered as False on a condition (this patch just check if the value is defined; e.g. is not None).

Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <git@osuv.de>
(cherry picked from commit 4f989bd)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Apr 21, 2022
[PR #1077/4f989bdd backport][stable-3] s3_lifecycle: support value '0' for transition_days

This is a backport of PR #1077 as merged into main (4f989bd).
SUMMARY
s3_lifecycle module does not support value 0 for transition_days parameter.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
s3_lifecycle
ADDITIONAL INFORMATION
A lifecycle rule with 0 as transition_days allows to create a rule that will move objects with a delta of just few hours (if set to 1, objects will be moved with a delta of 1d + few hours).
When the value 0 is set, the value is stripped from the query and we get an error that is hard to correlate with an "invalid"  value on this parameter (which is valid as 0 is an integer):
- name: Create s3 buckets lifecycle rules
  s3_lifecycle:
    ec2_url: "FIXME"
    region: "FIXME"
    aws_access_key: "FIXME"
    aws_secret_key: "FIXME"
    name: "FIXME"
    rule_id: "onezone"
    transitions:
    - transition_days: 0
      storage_class: "ONEZONE_IA"
    state: present
    status: enabled

fatal: [localhost]: FAILED! => {
    "boto3_version": "1.21.3",
    "botocore_version": "1.24.19",
    "changed": false,
    "error": {
        "code": "MalformedXML",
        "message": "Extra element Transition in interleave"
    },
    "lifecycle_configuration": {
        "Rules": [
            {
                "Filter": {
                    "Prefix": ""
                },
                "ID": "onezone",
                "Status": "Enabled",
                "Transitions": [
                    {
                        "StorageClass": "ONEZONE_IA"
                    }
                ]
            }
        ]
    },
    ...
MSG:

An error occurred (MalformedXML) when calling the PutBucketLifecycleConfiguration operation: Extra element Transition in interleave

This is because transition.get('transition_days') returns 0 which is considered as False on a condition (this patch just check if the value is defined; e.g. is not None).

Reviewed-by: Markus Bergholz <git@osuv.de>
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Apr 21, 2022
[PR #1077/4f989bdd backport][stable-2] s3_lifecycle: support value '0' for transition_days

This is a backport of PR #1077 as merged into main (4f989bd).
SUMMARY
s3_lifecycle module does not support value 0 for transition_days parameter.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
s3_lifecycle
ADDITIONAL INFORMATION
A lifecycle rule with 0 as transition_days allows to create a rule that will move objects with a delta of just few hours (if set to 1, objects will be moved with a delta of 1d + few hours).
When the value 0 is set, the value is stripped from the query and we get an error that is hard to correlate with an "invalid"  value on this parameter (which is valid as 0 is an integer):
- name: Create s3 buckets lifecycle rules
  s3_lifecycle:
    ec2_url: "FIXME"
    region: "FIXME"
    aws_access_key: "FIXME"
    aws_secret_key: "FIXME"
    name: "FIXME"
    rule_id: "onezone"
    transitions:
    - transition_days: 0
      storage_class: "ONEZONE_IA"
    state: present
    status: enabled

fatal: [localhost]: FAILED! => {
    "boto3_version": "1.21.3",
    "botocore_version": "1.24.19",
    "changed": false,
    "error": {
        "code": "MalformedXML",
        "message": "Extra element Transition in interleave"
    },
    "lifecycle_configuration": {
        "Rules": [
            {
                "Filter": {
                    "Prefix": ""
                },
                "ID": "onezone",
                "Status": "Enabled",
                "Transitions": [
                    {
                        "StorageClass": "ONEZONE_IA"
                    }
                ]
            }
        ]
    },
    ...
MSG:

An error occurred (MalformedXML) when calling the PutBucketLifecycleConfiguration operation: Extra element Transition in interleave

This is because transition.get('transition_days') returns 0 which is considered as False on a condition (this patch just check if the value is defined; e.g. is not None).

Reviewed-by: Markus Bergholz <git@osuv.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-2 PR should be backported to the stable-2 branch backport-3 PR should be backported to the stable-3 branch bug This issue/PR relates to a bug community_review integration tests/integration mergeit Merge the PR (SoftwareFactory) module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants