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 static 404 #593

Merged
merged 2 commits into from
Feb 27, 2019
Merged

Fix static 404 #593

merged 2 commits into from
Feb 27, 2019

Conversation

tripodsan
Copy link
Contributor

see #591

also adds action name to trace for better debugging of the request chain.

@tripodsan tripodsan requested a review from trieloff February 27, 2019 02:05
@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #593 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   85.41%   85.41%           
=======================================
  Files          39       39           
  Lines        1961     1961           
  Branches      300      300           
=======================================
  Hits         1675     1675           
  Misses        286      286

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b338328...7d4192f. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #593 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   85.41%   85.41%           
=======================================
  Files          39       39           
  Lines        1961     1961           
  Branches      300      300           
=======================================
  Hits         1675     1675           
  Misses        286      286

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b338328...7d4192f. Read the comment docs.

@trieloff trieloff requested a review from ejthurgo February 27, 2019 08:29
Copy link
Contributor

@trieloff trieloff left a comment

Choose a reason for hiding this comment

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

Ideally, we would cache the 404 coming from the static action, right?

@tripodsan
Copy link
Contributor Author

Ideally, we would cache the 404 coming from the static action, right?

yes, but not from the html action. I wouldn't invest too much here, as the refactored VCL works quite different. and, as I mentioned elsewhere, it would probably be better to only have 1 action that handles everything ;-)

@ejthurgo
Copy link
Collaborator

It's common to cache 404s, but usually quite short lifespans compared to 2XX so perhaps explicitly specify a shorter TTL.
But I'm tempted to agree with Tobias, maybe worth waiting until the new VCL merges.

@trieloff
Copy link
Contributor

Ok, I'm merging this, but putting it on the adobe/helix-publish#7 post-merge todo list.

@trieloff trieloff merged commit e4341d8 into master Feb 27, 2019
@tripodsan tripodsan deleted the fix-static-404 branch February 28, 2019 00:37
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