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

feat: add support for lifecycle rules in simple bucket #49

Merged
merged 7 commits into from
Apr 22, 2020
Merged

Conversation

umairidris
Copy link
Contributor

No description provided.

@umairidris umairidris requested a review from morgante April 22, 2020 01:00
@umairidris umairidris changed the title feat: add support for lifecycle rules feat: add support for lifecycle rules in simple bucket Apr 22, 2020
# Object with keys:
# - type - The type of the action of this Lifecycle Rule. Supported values: Delete and SetStorageClass.
# - storage_class - (Required if action type is SetStorageClass) The target Storage Class of objects affected by this Lifecycle Rule.
action = any
Copy link
Member

@bharathkkb bharathkkb Apr 22, 2020

Choose a reason for hiding this comment

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

nit: Are we switching to any to keep the end user configs more concise?
vs main module types

Copy link
Contributor Author

@umairidris umairidris Apr 22, 2020

Choose a reason for hiding this comment

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

Right, this is a temporary type and is due to the fact that terraform does not support optional keys in objects.

In the main module, I am guessing you always need to set storage_class on the action to empty or null, even if you only want to set type?

Once Terraform can support optional keys then we can describe the entire schema.

Note: if the user tries to use any other type it will cause an error so this is relatively safe. The only major downside is it won't catch any extra fields set in the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: also can't use map(string) without breaking the interface like how the main module forces matches_storage_class to be a string instead of list of strings like the underlying API. I believe any is better than map(string) because it lets you preserve the underlying API.

Copy link
Member

@bharathkkb bharathkkb Apr 22, 2020

Choose a reason for hiding this comment

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

yeah I agree. any lgtm, I was wondering if making a similar change to any type on the main module is also worthwhile, since it seems to be the best option as we wait for optional keys in obj.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to backporting this to the main module.

# Object with keys:
# - type - The type of the action of this Lifecycle Rule. Supported values: Delete and SetStorageClass.
# - storage_class - (Required if action type is SetStorageClass) The target Storage Class of objects affected by this Lifecycle Rule.
action = any
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to backporting this to the main module.

@morgante morgante merged commit b39e2cd into master Apr 22, 2020
@umairidris umairidris deleted the lifecycle branch April 22, 2020 19:52
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