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

Check if maxage header value is valid before setting it #3252

Conversation

senn
Copy link
Contributor

@senn senn commented Feb 1, 2024

Small but useful fix for issue #3251

I'm hoping this could get some fast reviews and approvals, and even a fast release. We could use this fix asap.

@senn senn force-pushed the improvement/check-valid-maxage-value branch from 9cb2de8 to 8a056c0 Compare February 1, 2024 14:31
@senn senn force-pushed the improvement/check-valid-maxage-value branch from 8a056c0 to 34e53ce Compare February 1, 2024 14:32
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (7b542e0) 1.34% compared to head (34e53ce) 1.34%.
Report is 9 commits behind head on master.

Files Patch % Lines
...egarbagecollector/PackageGarbageCollectionJob.java 0.00% 23 Missing ⚠️
.../main/java/com/adobe/acs/commons/data/Variant.java 0.00% 11 Missing ⚠️
...mpl/PropertyBasedDispatcherMaxAgeHeaderFilter.java 0.00% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             master   #3252      +/-   ##
===========================================
- Coverage      1.34%   1.34%   -0.01%     
  Complexity       44      44              
===========================================
  Files           721     721              
  Lines         29515   29522       +7     
  Branches       3837    3838       +1     
===========================================
  Hits            397     397              
- Misses        29102   29109       +7     
  Partials         16      16              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kwin kwin 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.

@henrykuijpers
Copy link
Contributor

Indeed, looks very good! Can this be merged quickly, would be nice to have it in the next release @kwin @davidjgonzalez

@davidjgonzalez
Copy link
Contributor

@senn can you add a Changelog entry?

@davidjgonzalez davidjgonzalez merged commit f53e9ad into Adobe-Consulting-Services:master Feb 2, 2024
19 of 21 checks passed
@senn
Copy link
Contributor Author

senn commented Feb 3, 2024

Thx for the fast actions, everybody!

@davidjgonzalez I see you have already added the changelog yourself. ✅

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.

4 participants