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

fix(sdk): set log retention for cloud.Function (tf-aws and awscdk) #4303

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

exoego
Copy link
Contributor

@exoego exoego commented Sep 27, 2023

Part of #4134

Added logRetentionDays property to cloud.Function.
It is implemented for tf-aws and awscdk.

I skipped tf-gcp and tf-azure since the ticket does not mention and other cloud provider appears to have default retention:

  • Cloud Logging: 30 days
  • Azure Monitor Application Insights: 90 days

Checklist

  • Title matches Winglang's style guide
  • Description explains motivation and solution
  • Tests added (always)
  • Docs updated (only required for features)
  • Added pr/e2e-full label if this feature requires end-to-end testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.

@monadabot monadabot added the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Sep 27, 2023
@exoego exoego marked this pull request as ready for review September 27, 2023 12:17
@exoego exoego requested a review from a team as a code owner September 27, 2023 12:17
@tsuf239 tsuf239 removed the ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! label Oct 1, 2023
Copy link
Collaborator

@tsuf239 tsuf239 left a comment

Choose a reason for hiding this comment

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

Is it possible add a wing test? (is there a way maybe an HTTP call to verify this property was set?)

libs/wingsdk/src/target-awscdk/function.ts Show resolved Hide resolved
@tsuf239
Copy link
Collaborator

tsuf239 commented Oct 1, 2023

also, when I think of it now, maybe it will be good to add a "global" log retention value (let's say I want to set any of my resources to 1 week retention period), which probably should be in a different PR
@skorfmann WDYT?

@tsuf239
Copy link
Collaborator

tsuf239 commented Oct 2, 2023

There is one test that fails on aws, maybe we need to merge the main branch?
image

@exoego
Copy link
Contributor Author

exoego commented Oct 3, 2023

@tsuf239 Rebased into the main

Copy link
Collaborator

@tsuf239 tsuf239 left a comment

Choose a reason for hiding this comment

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

Nice!
image

@mergify
Copy link
Contributor

mergify bot commented Oct 3, 2023

Thanks for contributing, @exoego! This PR will now be added to the merge queue, or immediately merged if log-retention is up-to-date with main and the queue is empty.

mergify bot added a commit that referenced this pull request Oct 3, 2023
@mergify mergify bot merged commit d7ceee4 into winglang:main Oct 3, 2023
15 checks passed
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.34.14.

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