Skip to content

Added back CacheControl headers from PR #6265#9549

Closed
Bro-Chain wants to merge 3 commits intotendermint:v0.34.xfrom
Bro-Chain:v0.34.x
Closed

Added back CacheControl headers from PR #6265#9549
Bro-Chain wants to merge 3 commits intotendermint:v0.34.xfrom
Bro-Chain:v0.34.x

Conversation

@Bro-Chain
Copy link

@Bro-Chain Bro-Chain commented Oct 13, 2022

Issue #5764 raised the feature request of having the RPC return Cache-Control headers when applicable. This resulted in PR #6265, which was later merged. But it was merged into master, so it never made it to release in 0.34.x. This is that patch again, but so it can actually be released.


Closes #5764 (again)

PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@Bro-Chain Bro-Chain requested a review from a team October 13, 2022 18:23
@JayT106
Copy link
Contributor

JayT106 commented Oct 18, 2022

link to PR #9442

}

// Set the default response cache to true unless
// 1. Any RPC request rrror.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 1. Any RPC request rrror.
// 1. Any RPC request error.

sergio-mena
sergio-mena previously approved these changes Oct 18, 2022
Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I added one more minor comment

@sergio-mena
Copy link
Contributor

Please add a changelog entry

}
w.Header().Set("Content-Type", "application/json")
if c {
w.Header().Set("Cache-Control", "public, max-age=86400") // expired after one year
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it now one day?

@sergio-mena
Copy link
Contributor

link to PR #9442

I took a look at #9442. My understanding is that #9442 is equivalent to this PR (but for main, whereas this is for v0.34.x). So I have the following questions:

@thanethomson
Copy link
Contributor

This PR is a duplicate of #9442.

@sergio-mena sergio-mena dismissed their stale review October 21, 2022 20:30

Following this and this, we believe this should be closed

@thanethomson thanethomson mentioned this pull request Oct 31, 2022
3 tasks
@thanethomson
Copy link
Contributor

Supeseded by #9650.

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