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

Serve local client-config.json if it exists #1038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthew-white
Copy link
Member

@matthew-white matthew-white commented Oct 15, 2024

Now that we request client-config.json, I've been wondering whether it'd be possible to remove npm run dev:oidc. That command sets a single option in the config (oidcEnabled), but there are several other options in the config that a developer might want to set. The idea of this PR is that a developer can create a local client-config.json to match whatever they're working on, and those options won't show up in version control. Modifying src/config.js would have much the same effect, but it has the downside that it shows up in version control.

Related: #1034

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@@ -95,7 +95,10 @@ http {
}

location = /client-config.json {
include ./common-headers.nginx.conf;
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't call add_header in this location, so I don't think we need to include this file. Locally, I can see that the expected headers are coming through.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this change be a separate PR? It seems to be unrelated to the aim of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep this line, as it doesn't do any harm, and keeps this file closer to central's files/nginx/odk.conf.template, and therefore easier to compare.

We could go further in mirroring central's files/nginx/odk.conf.template by adding:

add_header Cache-Control no-cache;

@alxndrsn
Copy link
Contributor

I like this approach because it's more consistent with how production behaves.

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.

2 participants