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: adds table_expiration_days and partition_expiration_days #143

Conversation

geojaz
Copy link
Contributor

@geojaz geojaz commented Feb 6, 2023

feat!: Adds partition_expiration_days

Closes #142

Nothing too complicated here, just adding a field per request of customer.

I will submit a second PR to deprecate expiration_days in favor of table_expiration_days for clarity that can be merged when we have a batch of breaking changes.

@geojaz geojaz requested a review from a team as a code owner February 6, 2023 18:59
@bharathkkb
Copy link
Member

/gcbrun

@geojaz geojaz force-pushed the add_default_partition_expiration_ms branch from 6ef5a90 to d1ec4c7 Compare February 6, 2023 23:04
Adds table_expiration_days and partition_expiration_days.

BREAKING CHANGE: deprecrates expiration_days for clarity.
@geojaz geojaz force-pushed the add_default_partition_expiration_ms branch from d1ec4c7 to 27567ac Compare February 6, 2023 23:07
@geojaz
Copy link
Contributor Author

geojaz commented Feb 6, 2023

@bharathkkb How do i fix this commit message? I checked out the docs and it looks like i'm fairly close. any advice?

@apeabody
Copy link
Contributor

apeabody commented Feb 6, 2023

/gcbrun

@apeabody
Copy link
Contributor

apeabody commented Feb 7, 2023

@bharathkkb How do i fix this commit message? I checked out the docs and it looks like i'm fairly close. any advice?

Hi @geojaz - Thanks of the contribution. It appears you need to update the PR's title which is currently "Adds table_expiration_days and partition_expiration_days; deprecates expiration_days". The first line of your PR's description looks like it would probably work.

@comment-bot-dev
Copy link

@geojaz
Thanks for the PR! 🚀
✅ Lint checks have passed.

@geojaz geojaz changed the title Adds table_expiration_days and partition_expiration_days; deprecates expiration_days feat!: adds table_expiration_days and partition_expiration_days Feb 7, 2023
@geojaz
Copy link
Contributor Author

geojaz commented Feb 7, 2023

duh, thanks :)

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

@geojaz Thanks for the PR! Let's preserve expiration_days for now just to prevent a breaking change and instead open an issue to bundle this in with our next breaking change.

Adds partition_expiration_days_ms.
I've split out the mutation of the expiration_days param; I'll add a
tracker issue and another PR that can be incorporated when we're ready to
batch with other breaking changes.
@geojaz geojaz force-pushed the add_default_partition_expiration_ms branch from 00c3fb7 to 2b77260 Compare February 8, 2023 18:03
@geojaz geojaz changed the title feat!: adds table_expiration_days and partition_expiration_days WIP feat!: adds table_expiration_days and partition_expiration_days Feb 8, 2023
@geojaz geojaz force-pushed the add_default_partition_expiration_ms branch from 2b77260 to 8f1e978 Compare February 8, 2023 19:03
@geojaz geojaz changed the title WIP feat!: adds table_expiration_days and partition_expiration_days feat: adds table_expiration_days and partition_expiration_days Feb 8, 2023
@geojaz geojaz force-pushed the add_default_partition_expiration_ms branch from 8f1e978 to bc27238 Compare February 8, 2023 19:06
@geojaz
Copy link
Contributor Author

geojaz commented Feb 8, 2023

@bharathkkb Sounds good... this has been updated to keep expiration_days for now... I'll add an issue and put a PR in that can be adopted when the time is right.

@bharathkkb
Copy link
Member

/gcbrun

@geojaz
Copy link
Contributor Author

geojaz commented Feb 14, 2023

@bharathkkb any other feedback on this one or are we good to go?

@bharathkkb bharathkkb merged commit 42876c4 into terraform-google-modules:master Feb 23, 2023
@bharathkkb
Copy link
Member

@geojaz sorry for the delay just merged!

@geojaz geojaz deleted the add_default_partition_expiration_ms branch February 23, 2023 16:20
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.

Support default_partition_expiration_ms on bq datasets
4 participants