required changes for local setup #1

davidverdu · 2025-01-28T18:02:44Z

As the FE is build process generates static CSS and JS, configuration parameters in the form of environment variables need to be available during build time. As well, there is a configuration check that runs within the application to verify that the expected configuration parameters are available to the application.

There are different approaches to address this issue so that the application has the right configuration for the environment that is going to be deployed into. The chosen approach is to build the application as part of the runtime startup script and pass the configuration parameters as environment variables at runtime.

Changes

creatd startup script: entrypoint.sh to execute build and run the application.
Refactored Dockerfile to remove build step.
Refactored how env variables are loaded to consolidation into a single method from the Vite framework.
Removed env module as this is not necessary after refactor.
Removed loading of .env.okta properties and loading mechanism as this is not necessary after refactor.
Updated vite.config.js to make it closer to the version from main branch.

Notes:

The changes to the env variables in this PR will require the parent project to be updated to use ./okta-linking-frontend/.env in Docker Compose

davidverdu · 2025-01-28T18:03:33Z

.dockerignore

-#!.development.env
+!.development.env


README says that this line should be uncommented to make it work locally

At runtime, not to be committed. README also says DO NOT COMMIT SECRETS HERE. Secrets management did not exist, so this was a first pass. Let's discuss options for secrets management that does not involve committing them to the repo.

Dockerfile

davidverdu · 2025-01-28T18:05:10Z

env/env.js

-const ENV_FILE = '.production.env';
+const ENV_FILE = '.development.env';


README says that this change should be applied to make it work locally. Without this change, docker compose was not working properly

At runtime, not to be committed. I'm getting the impression that the kludge I made for secrets management works but is non-intuitive. Let's discuss how this could be made better.

OK. I think that the code of env/env.js is complicating how env variables are passed. I believe that declaring a .env file is enough to make dotenv load the variables

I agree that seemed peculiar to me also and I was not certain of the developer's intentions. It would be better if we could do this in a more standard way.

whorka

The change to the Dockerfile looks good but the changes to secrets management are not appropriate. Please rescind them and let's talk about a better way to do secrets management.

davidverdu · 2025-01-29T13:35:50Z

I left the PR only with the Dockerfile bug fix. Let's discuss the secrets management.

entrypoint.sh

davidverdu · 2025-02-04T14:45:24Z

vite.config.js


-dns.setDefaultResultOrder('verbatim')
+dotenv.config()


The env variables are already loaded from memory with process.env. However, I left this clause in case users wanted to run the frontend without Docker, so that a .env file can be used in this folder

Please revert this change to the behavior in the main branch. If the behavior in the main branch does not break any required functionality, we will want to keep the code as close as possible to main so that we can continuously pull in upstream changes easily. Lets limit ourselves to only adding required functionality.

Dockerfile

whorka

Small change requested but overall this is a big improvement. Thank you!

whorka · 2025-02-04T16:50:03Z

.dockerignore

This change is inconsequential; please remove it.

re-committing this to remove .development.env

Dockerfile

entrypoint.sh

whorka · 2025-02-04T16:54:03Z

vite.config.js


-dns.setDefaultResultOrder('verbatim')
+dotenv.config()


Please revert this change to the behavior in the main branch. If the behavior in the main branch does not break any required functionality, we will want to keep the code as close as possible to main so that we can continuously pull in upstream changes easily. Lets limit ourselves to only adding required functionality.

whorka

Looks even better than before, but I still think we can bring it even closer into line with upstream. Would you be willing to make one more pass on this Aday?

whorka · 2025-02-06T20:01:22Z

.okta.env

Why delete this file and load the environment variables from a .env file? Why not just load this file like the main branch does?

whorka · 2025-02-06T20:02:13Z

README.md

@@ -6,10 +6,11 @@ Notes:

 Amendments:
 * Configuration:
-  1. Copy `.example.env` to `.development.env` and fill in values.
-  2. Edit `env/env.js` to load `.development.env` instead of production env (FIXME: this should be paramaterized).
+  1. Copy `.example.env` to `.env` and fill in values.


As above, why is this file now named .env instead of .okta.env?

vite.config.js

whorka · 2025-02-06T20:14:17Z

vite.config.js

+    customEnv[key] = env[key]
+  })
+
+  dns.setDefaultResultOrder('verbatim')


Can you see any reason why our Okta PS consultant added this? It appears superfluous since it is setting the default option and should only make a difference if IPv6 is misconfigured. https://nodejs.org/api/dns.html#dnssetdefaultresultorderorder

I'd prefer we remove this and

import dns from 'dns'

if they are not needed, as long as we are cleaning up this file.

Looking into this...

Looking at the Vite docs, this option is to configure the server that runs Vite and serves the React app to the user.
Seems that there are cases that if the configured domain to server the app resolves to multiple IPs (like localhost), Vite might not serve the request if there is a mismatch between Browser and server. This dns fix is used to address it. From the docs: https://vite.dev/config/server-options

I tested without and it works as before. Not sure it was happening to the Okta PS consultant.

We can remove and test the results in an environment to see if is necessary

This is now removed

abujeda · 2025-02-10T14:48:56Z

Sorry for the delay but I didn't get any notifications. I just realized that because I only updated the description and haven't made any comments yet, I wasn't in the list of recipients for notifications.

Looking into the comments and making the necessary updates

whorka

I reviewed and recommitted .dockerignore (removing .development.env) and .okta.env (restoring from upstream). Aside from that there are just a couple small documentation fixes. I tested and it works fine. Please commit the doc changes if you agree and this can be merged.

README.md

.example.env

whorka · 2025-02-11T17:28:45Z

.dockerignore

re-committing this to remove .development.env

whorka · 2025-02-11T17:35:55Z

If you plan to test you will need to pull hmdc/okta-linking-app:main since I committed an update there to read okta-linking-frontend/.okta.env instead of okta-linking-frontend/.development.env

Co-authored-by: William Horka <[email protected]>

abujeda · 2025-02-11T18:10:11Z

Changes looks good!! I have merged the changes.

This is ready to merge to main

whorka · 2025-02-11T18:23:03Z

Changes looks good!! I have merged the changes.

This is ready to merge to main

Thank you! Let's merge to linkingapp for now. I intend to rebase all of linkingapp on top of okta-samples/okta-react-sample:main

required changes

337f8f2

davidverdu commented Jan 28, 2025

View reviewed changes

Dockerfile Show resolved Hide resolved

davidverdu commented Jan 28, 2025

View reviewed changes

davidverdu self-assigned this Jan 28, 2025

davidverdu requested a review from whorka January 28, 2025 18:23

whorka requested changes Jan 28, 2025

View reviewed changes

revert changes to make it work locally

b51c2bb

davidverdu requested a review from whorka January 29, 2025 13:36

davidverdu added 11 commits February 3, 2025 17:24

inject env variables

7bccff9

add config

6935556

rename env variables in code

9092ad6

fix config file

c2a3524

remove test variable

47f4ed6

enhance dockerfile and entrypoint

c2fa527

remove prefix and use process.env

1031c81

remove cat from file

0a159b2

restore name of pending vars

d57fbd0

remove unneeded code from entrypoint.sh

d655789

enhance Dockerfile comments

0be0dbc

davidverdu commented Feb 4, 2025

View reviewed changes

entrypoint.sh Show resolved Hide resolved

davidverdu added 2 commits February 4, 2025 14:38

delete env.js file

1472c4f

undo Home page change for debugging

6196e3d

davidverdu commented Feb 4, 2025

View reviewed changes

Dockerfile Show resolved Hide resolved

whorka requested changes Feb 4, 2025

View reviewed changes

davidverdu added 3 commits February 4, 2025 18:05

mimic the most possible the version present in main branch

c74b46a

undo change on .dockerignore

691788f

fix vite.config.js

d1cc6e4

Removed inconsistent dotenv loading of environment variables

08ebef2

whorka requested changes Feb 6, 2025

View reviewed changes

abujeda and others added 4 commits February 10, 2025 22:00

Removed dns default order call

c1795b3

Added .okta.env properties loading with dotenv

85175f9

remove .development.env

cba73d8

recommit from upstream main

dbcf756

whorka requested changes Feb 11, 2025

View reviewed changes

abujeda and others added 2 commits February 11, 2025 18:02

Update README.md

6d79323

Co-authored-by: William Horka <[email protected]>

Update .example.env

d7cf9a7

Co-authored-by: William Horka <[email protected]>

whorka merged commit 4e08e4b into linkingapp Feb 11, 2025
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

required changes for local setup #1

required changes for local setup #1

davidverdu commented Jan 28, 2025 •

edited by abujeda

Loading

davidverdu Jan 28, 2025

whorka Jan 28, 2025

davidverdu Jan 28, 2025

whorka Jan 28, 2025

davidverdu Jan 29, 2025

whorka Jan 29, 2025

whorka left a comment

davidverdu commented Jan 29, 2025

davidverdu Feb 4, 2025

whorka Feb 4, 2025

whorka left a comment

whorka Feb 4, 2025

davidverdu Feb 5, 2025

whorka Feb 11, 2025

whorka Feb 4, 2025

whorka left a comment

whorka Feb 6, 2025

whorka Feb 6, 2025

whorka Feb 6, 2025

abujeda Feb 10, 2025

abujeda Feb 10, 2025

abujeda Feb 10, 2025

abujeda commented Feb 10, 2025 •

edited

Loading

whorka left a comment

whorka Feb 11, 2025

whorka commented Feb 11, 2025

abujeda commented Feb 11, 2025 •

edited

Loading

whorka commented Feb 11, 2025

		const ENV_FILE = '.production.env';
		const ENV_FILE = '.development.env';

		#!.development.env
		!.development.env


		dns.setDefaultResultOrder('verbatim')
		dotenv.config()


		dns.setDefaultResultOrder('verbatim')
		dotenv.config()

required changes for local setup #1

required changes for local setup #1

Conversation

davidverdu commented Jan 28, 2025 • edited by abujeda Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whorka left a comment

Choose a reason for hiding this comment

davidverdu commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whorka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whorka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abujeda commented Feb 10, 2025 • edited Loading

whorka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whorka commented Feb 11, 2025

abujeda commented Feb 11, 2025 • edited Loading

whorka commented Feb 11, 2025

davidverdu commented Jan 28, 2025 •

edited by abujeda

Loading

abujeda commented Feb 10, 2025 •

edited

Loading

abujeda commented Feb 11, 2025 •

edited

Loading