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

Add messageRetentionDuration to PubSub Topic #440

Merged

Conversation

Feggah
Copy link
Collaborator

@Feggah Feggah commented Jun 11, 2022

Description of your changes

Fixes #422

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • Managing the messageRetentionDuration field using the managed resource
  • Adding tests

Important note for reviewers

Currently, it is possible to use a fractional number in the messageRetentionDuration field, for example: 1200.5s.

This will cause an update-loop in the managed resource, because the API returns the value as 1200.500s, which is "different" in our comparisons.

This bug won't appear if the platform builder uses an integer, for example, 1200s. In this case, the GCP API will return 1200s and it will be considered up to date.

I decided to not include this parser to include zeros/exclude zeros after the dot before the comparisons for simplicity for now, but if you think it is crucial for this feature, please request changes :)

@Feggah Feggah requested a review from turkenh June 11, 2022 19:18
@turkenh
Copy link
Contributor

turkenh commented Sep 28, 2022

Currently, it is possible to use a fractional number in the messageRetentionDuration field, for example: 1200.5s.

This will cause an update-loop in the managed resource, because the API returns the value as 1200.500s, which is "different" in our comparisons.

This bug won't appear if the platform builder uses an integer, for example, 1200s. In this case, the GCP API will return 1200s and it will be considered up to date.

I decided to not include this parser to include zeros/exclude zeros after the dot before the comparisons for simplicity for now, but if you think it is crucial for this feature, please request changes :)

I don't think it is crucial, but also I believe we can just parse them as duration and compare https://go.dev/play/p/wKfeyJxpOIZ ?

@Feggah Feggah force-pushed the feature/message-retention-duration branch from 456dae4 to 3a905d6 Compare October 1, 2022 19:40
@Feggah
Copy link
Collaborator Author

Feggah commented Oct 1, 2022

I don't think it is crucial, but also I believe we can just parse them as duration and compare https://go.dev/play/p/wKfeyJxpOIZ ?

Sure, I didn't think about this solution @turkenh. Thanks for the tip!

Added the suggestion 🙂

@Feggah Feggah mentioned this pull request Oct 3, 2022
Copy link
Contributor

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Feggah!

Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah Feggah force-pushed the feature/message-retention-duration branch from 3a905d6 to 1ea920f Compare October 10, 2022 16:44
@Feggah Feggah merged commit 8d58196 into crossplane-contrib:master Oct 10, 2022
@Feggah Feggah deleted the feature/message-retention-duration branch October 10, 2022 16: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.

GCP provider pubsub topic message retention
2 participants