Make the default time durations configurable on User Form Interface #212

rktoomey · 2024-12-11T21:04:16Z

This is a rewrite of #194 after the discussion in the Discord channel at https://discord.com/channels/1232815453907189922/1306560215130443827.

This PR creates a default configuration file for the access app, and allows an environment-specific override configuration file path to be supplied as a build arg to Docker. See the README for details.

During the build process, craco uses webpack Define plugin to make the access config contents globally available as part of the static build artifacts.

cc @jonathanhle

savathoon · 2024-12-11T21:47:40Z

🙏 this is incredible work thank you so much for fleshing out this proof of concept for dynamically loaded configs.

I have some questions about specific parts of the approach, I don't consider these blocking concerns but am curious about some specific decisions around loading the config from the static folder instead of pre-populating the js with config values. I'll add more detail around specific implementations but this round trip to retrieve config values is my biggest current open question.

savathoon · 2024-12-11T21:50:48Z

craco.config.js

@@ -15,5 +17,23 @@ module.exports = {
    alias: {
      '@mui/styled-engine': '@mui/styled-engine-sc',
    },
+    plugins: [
+      new CopyWebpackPlugin({


It's been awhile since I've looked at webpack plugins, but I think instead of doing a blind copy of the config file into the build dir it might be possible to populate config values server-side and generate a "config" module that has the right values pre-populated from something like an *.env file. The benefits of doing it this way would be to save a round trip and make the initial time to render faster but if there are cons to this approach happy to discuss.

We want to be able to configure the access time dropdowns. The first PR (#194) used environment variables in .env.production. My attempt uses a config file because in the discord discussion (https://discord.com/channels/1232815453907189922/1306560215130443827) the proposed revision was to use a config file that would be copied in with webpack.

populate config values server-side

Unfortunately, I don't understand what you want :(
I am not well-versed in React so that might be the problem.

@savathoon please see my latest commit.

removed config files and config loader

create access_config.py to create a typed AccessConfig instance, looking for env vars that override individual properties.

created Flask endpoint in app.py to serve /access_config.js

use public/index.html to load the /access_config.js script in every page

updated the front-end config loader to use window.__ACCESS_CONFIG__ as the source of the config

updated the callsites

Please see what you think, because I'm not sure that I understand what you want.

somethingnew2-0

We shouldn't include a roundtrip in every page load to load static config. This static config should be included in the JS bundle during build time.

rktoomey · 2024-12-12T15:41:50Z

@somethingnew2-0 @savathoon

I believe I've made the changes you requested. In this latest iteration. Please let me know what you think.

craco.config.js

rktoomey · 2024-12-12T21:19:09Z

Ok. Now you can set a build arg for Docker to use the config override file specified at ACCESS_FILE_CONFIG_PATH.

.env.production.example

somethingnew2-0 · 2024-12-13T22:15:51Z

This is looking much better, thanks for adding this!

I'm going to let @savathoon give it another review when they get a chance before I approve

docker-compose.yml

…e path supplied as a build arg to Docker

rktoomey · 2024-12-17T17:04:10Z

@savathoon do you have some time to look at this PR this week?

jonathanhle · 2024-12-29T23:33:26Z

@savathoon do you have some time to look at this PR this week?

Let us know if there's anything additional to get this merged upstream.

savathoon

Sorry for the late turnaround, thank you for making the previous round of changes. Most of what I suggested is in the interest of simplification/consolidation but would love to get your thoughts.

Dockerfile

.env.production.example

src/config/config.empty.json

src/config/accessConfig.ts

rktoomey · 2025-01-03T04:05:27Z

Sorry for the late turnaround, thank you for making the previous round of changes. Most of what I suggested is in the interest of simplification/consolidation but would love to get your thoughts.

Before I do any further work on this, I'd like to confirm that @somethingnew2-0 agrees with the changes requested in this review.

Further, I'd like us to agree that the override config file will be kept in src/config. This way there can be an env var in .env.production without the need for a build arg to Docker to ensure that the override config file gets copied to the Docker image before craco runs.

cc @jonathanhle

somethingnew2-0 · 2025-01-03T20:01:00Z

Before I do any further work on this, I'd like to confirm that @somethingnew2-0 agrees with the changes requested in this review.

I've reviewed @savathoon's comments, they suggested to me privately taking at forking and suggesting a few commits to simplify things.

Further, I'd like us to agree that the override config file will be kept in src/config. This way there can be an env var in .env.production without the need for a build arg to Docker to ensure that the override config file gets copied to the Docker image before craco runs.

That sounds good to me. IMO simplicity is preferred. So if we can remove the need for a build arg in the Dockerfile, all the better.

1. Move config files to project-level config directory 2. Move logic out of craco.config.js to src/config/loadAccessConfig.js 3. Update README

jonathanhle · 2025-01-14T00:19:30Z

README.md

+
+To override values on the back-end, modify these key-value pairs inside the `BACKEND` key in your custom config file.
+
+_There are currently no supported back-end configuration overrides but this is planned to change soon._


I'll be working on a follow-on PR, where I need to override some strings on the backend related to the regex name pattern used for Okta Group names.

jonathanhle · 2025-01-14T00:20:17Z

config/config.default.json

+    },
+    "DEFAULT_ACCESS_TIME": "1209600"
+  },
+  "BACKEND": {}


I'll be working on a follow-on PR, where I need to override some strings on the backend related to the regex name pattern used for Okta Group names.

Actually, I added that in d1968e5 to show why having the same config file for front-end and back-end is useful (much like how we use .env.production for env vars in both React and Flask)

rktoomey · 2025-01-14T12:43:04Z

@somethingnew2-0 @savathoon Review requested again.

As requested, the need for the Docker build arg is removed.
craco.config.js is tidied up: the logic for the access config load and validation has been moved to src/config/loadAccessConfig.js. Why js and not ts? Well....

I tried to make the change @savathoon suggested, to convert the config to a Typescript object. This failed for multiple reasons:

craco.config.js is CommonJS and can't deal with Typescript
attempting to type the object to AccessConfig in globals.d.ts fails because it can't import that type from src/config/accessConfig.ts
trying to covert the craco config to Typescript opened up a whole bunch of other problems, starting with that react-app-alias doesn't publish types

Whilst I was doing that, @jonathanhle and I spoke about his need to be able to configure the Access app on both the front-end and the back-end. His use case is simple: our pre-existing Okta entities use a different pattern to the one hard-coded in the Flask and React app.

So keeping the access config file JSON gives us the flexibility to do this neatly: now you can specify a pattern and a custom error message. I also wrote an access config file loader and tests.

Please re-review.

- add and test access config loader in Flask back-end app - add sample configuration both both front- and back-end: extract name validation pattern to config - update README

jonathanhle · 2025-01-17T05:46:27Z

@somethingnew2-0 @savathoon Review requested again.

As requested, the need for the Docker build arg is removed. craco.config.js is tidied up: the logic for the access config load and validation has been moved to src/config/loadAccessConfig.js. Why js and not ts? Well....

I tried to make the change @savathoon suggested, to convert the config to a Typescript object. This failed for multiple reasons:

craco.config.js is CommonJS and can't deal with Typescript

attempting to type the object to AccessConfig in globals.d.ts fails because it can't import that type from src/config/accessConfig.ts

trying to covert the craco config to Typescript opened up a whole bunch of other problems, starting with that react-app-alias doesn't publish types

Whilst I was doing that, @jonathanhle and I spoke about his need to be able to configure the Access app on both the front-end and the back-end. His use case is simple: our pre-existing Okta entities use a different pattern to the one hard-coded in the Flask and React app.

So keeping the access config file JSON gives us the flexibility to do this neatly: now you can specify a pattern and a custom error message. I also wrote an access config file loader and tests.

Please re-review.

Any chance this approach would work? It solves a need for overrides on the frontend and backend and keeps the related overrides in the same spot. We can maintain a slightly departed fork too for our needs, was just trying to avoid that.

somethingnew2-0 · 2025-01-22T00:49:13Z

This PR is looking good to me, I love how we can specify/validate the same config for both frontend and backend. ❤️

The current python segmentation fault test failure is unrelated and something I'm trying to track down with Github support. Tests pass locally for me.

Totally understand the issue of not being able to use Typescript during buildtime in the craco config.

I'm going to let @savathoon give a final pass for any opinions, but to me this is looking ready to ship.

jonathanhle

LGTM. Works in my local testing.

savathoon

Looks good! Some open questions but nothing that's a blocking concern, will sanity check locally.

somethingnew2-0 · 2025-01-23T00:51:03Z

We fixed the unrelated test failures in #229

Could you rebase or merge from main, so we can run the tests and then merge if they pass?

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch from 1ee1dee to d4aacac Compare December 11, 2024 21:14

savathoon reviewed Dec 11, 2024

View reviewed changes

jonathanhle mentioned this pull request Dec 11, 2024

Make the default time durations configurable on User Form Interface #194

Closed

rktoomey changed the title ~~Add front-end config file with defaults and environmental override~~ Make the default time durations configurable on User Form Interface Dec 12, 2024

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch 2 times, most recently from be05a70 to 465439c Compare December 12, 2024 13:15

somethingnew2-0 requested changes Dec 12, 2024

View reviewed changes

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch 2 times, most recently from d4aacac to 4f29c36 Compare December 12, 2024 15:40

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch from 4f29c36 to aa01f11 Compare December 12, 2024 15:43

rktoomey requested a review from somethingnew2-0 December 12, 2024 17:56

somethingnew2-0 reviewed Dec 12, 2024

View reviewed changes

craco.config.js Outdated Show resolved Hide resolved

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch from aa01f11 to 8bdff73 Compare December 12, 2024 21:17

rktoomey requested a review from somethingnew2-0 December 12, 2024 21:18

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch 2 times, most recently from 330a2d1 to b5e2b66 Compare December 13, 2024 21:22

somethingnew2-0 reviewed Dec 13, 2024

View reviewed changes

.env.production.example Outdated Show resolved Hide resolved

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch from b5e2b66 to e735f5c Compare December 13, 2024 22:26

somethingnew2-0 reviewed Dec 13, 2024

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

Make time dropdowns user configurable via an environmental config fil…

28237c2

…e path supplied as a build arg to Docker

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch from e735f5c to 28237c2 Compare December 14, 2024 10:33

somethingnew2-0 self-requested a review December 16, 2024 19:11

rktoomey requested a review from savathoon December 18, 2024 17:02

savathoon reviewed Jan 3, 2025

View reviewed changes

Dockerfile Show resolved Hide resolved

.env.production.example Show resolved Hide resolved

src/config/config.empty.json Outdated Show resolved Hide resolved

src/config/accessConfig.ts Outdated Show resolved Hide resolved

Move access config load logic out of craco config

193d08a

1. Move config files to project-level config directory 2. Move logic out of craco.config.js to src/config/loadAccessConfig.js 3. Update README

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch from c54be0a to d1968e5 Compare January 13, 2025 16:43

jonathanhle reviewed Jan 14, 2025

View reviewed changes

rktoomey requested a review from savathoon January 14, 2025 12:44

Make access config available on both front and back ends

9c09df1

- add and test access config loader in Flask back-end app - add sample configuration both both front- and back-end: extract name validation pattern to config - update README

rktoomey force-pushed the rt-PLATSEC-3456-access-config-3 branch from d1968e5 to 9c09df1 Compare January 14, 2025 15:53

Merge branch 'discord:main' into rt-PLATSEC-3456-access-config-3

caee8fd

jonathanhle approved these changes Jan 22, 2025

View reviewed changes

savathoon approved these changes Jan 22, 2025

View reviewed changes

somethingnew2-0 approved these changes Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the default time durations configurable on User Form Interface #212

Make the default time durations configurable on User Form Interface #212

rktoomey commented Dec 11, 2024 •

edited

Loading

savathoon commented Dec 11, 2024

savathoon Dec 11, 2024

rktoomey Dec 11, 2024

rktoomey Dec 12, 2024

somethingnew2-0 left a comment

rktoomey commented Dec 12, 2024

rktoomey commented Dec 12, 2024

somethingnew2-0 commented Dec 13, 2024

rktoomey commented Dec 17, 2024

jonathanhle commented Dec 29, 2024

savathoon left a comment

rktoomey commented Jan 3, 2025

somethingnew2-0 commented Jan 3, 2025

jonathanhle Jan 14, 2025 •

edited

Loading

jonathanhle Jan 14, 2025

rktoomey Jan 14, 2025

rktoomey commented Jan 14, 2025

jonathanhle commented Jan 17, 2025

somethingnew2-0 commented Jan 22, 2025

jonathanhle left a comment

savathoon left a comment

somethingnew2-0 commented Jan 23, 2025


		To override values on the back-end, modify these key-value pairs inside the `BACKEND` key in your custom config file.

		_There are currently no supported back-end configuration overrides but this is planned to change soon._

Make the default time durations configurable on User Form Interface #212

Are you sure you want to change the base?

Make the default time durations configurable on User Form Interface #212

Conversation

rktoomey commented Dec 11, 2024 • edited Loading

savathoon commented Dec 11, 2024

savathoon Dec 11, 2024

Choose a reason for hiding this comment

rktoomey Dec 11, 2024

Choose a reason for hiding this comment

rktoomey Dec 12, 2024

Choose a reason for hiding this comment

somethingnew2-0 left a comment

Choose a reason for hiding this comment

rktoomey commented Dec 12, 2024

rktoomey commented Dec 12, 2024

somethingnew2-0 commented Dec 13, 2024

rktoomey commented Dec 17, 2024

jonathanhle commented Dec 29, 2024

savathoon left a comment

Choose a reason for hiding this comment

rktoomey commented Jan 3, 2025

somethingnew2-0 commented Jan 3, 2025

jonathanhle Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

jonathanhle Jan 14, 2025

Choose a reason for hiding this comment

rktoomey Jan 14, 2025

Choose a reason for hiding this comment

rktoomey commented Jan 14, 2025

jonathanhle commented Jan 17, 2025

somethingnew2-0 commented Jan 22, 2025

jonathanhle left a comment

Choose a reason for hiding this comment

savathoon left a comment

Choose a reason for hiding this comment

somethingnew2-0 commented Jan 23, 2025

rktoomey commented Dec 11, 2024 •

edited

Loading

jonathanhle Jan 14, 2025 •

edited

Loading