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: ensure contentBusId is loaded properly #342

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Conversation

tripodsan
Copy link
Contributor

@tripodsan tripodsan commented Jul 1, 2023

  • remove contentBusId from PipelineStateOptions (constructor)
  • ensure that contentBusId is loaded from helix-config in all pipes
  • remove fake contentBusIds from state in tests

@github-actions
Copy link

github-actions bot commented Jul 1, 2023

This PR will trigger a patch release when merged.

@codecov
Copy link

codecov bot commented Jul 1, 2023

Codecov Report

Merging #342 (69b2114) into main (c6dd583) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #342   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           52        52           
  Lines         4308      4329   +21     
=========================================
+ Hits          4308      4329   +21     
Impacted Files Coverage Δ
src/PipelineState.js 100.00% <ø> (ø)
src/json-pipe.js 100.00% <100.00%> (ø)
src/options-pipe.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -98,7 +98,6 @@ async function computeSurrogateKeys(path, contentBusId) {
export async function jsonPipe(state, req) {
const { log } = state;
state.type = 'json';
const { contentBusId } = state;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefan-guggisberg this was the cause of the wrong surrogate key

await fetchConfigAll(state, request, response);
await setCustomResponseHeaders(state, request, response);
try {
await fetchConfig(state, request);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was completely missing! OPTIONS requests would have never worked properly

@@ -46,7 +46,6 @@ declare interface PipelineOptions {
ref: string;
partition: string;
path: string;
contentBusId: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove it from options, so that it isn't set accidentally

@tripodsan tripodsan merged commit 80e4552 into main Jul 4, 2023
@tripodsan tripodsan deleted the cbid-surrogate branch July 4, 2023 05:58
github-actions bot pushed a commit that referenced this pull request Jul 4, 2023
## [3.11.18](v3.11.17...v3.11.18) (2023-07-04)

### Bug Fixes

* ensure contentBusId is loaded properly ([#342](#342)) ([80e4552](80e4552))
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

🎉 This PR is included in version 3.11.18 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants