Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

Allow an optional static bearer token when using dev server #983

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Aug 24, 2022

When running on dev environment, sometimes it's useful to allow to provide static bearer token.
For example if running the UI with minikube[1]

In my use case I start minishift with a dev user that has some bearer token, and then run

AUTH_REQUIRED=false yarn start:dev:remote

with this patch the dev server command will look something like:

K8S_AUTH_BEARER_TOKEN=31ada4fd-adec-460c-809a-9e56ceb75269 AUTH_REQUIRED=false yarn start:dev:remote

[1] https://github.com/konveyor/forklift-console-plugin/blob/main/scripts/configure-minikube.sh

@yaacov yaacov requested a review from a team August 24, 2022 10:51
@yaacov
Copy link
Member Author

yaacov commented Aug 24, 2022

@mturley @sjd78 @rszwajko please review

@konveyor-preview-bot
Copy link

🚀 Deployed Preview: http://konveyor-forklift-ui-pr-983-preview.surge.sh

Compare with current main branch: http://konveyor-forklift-ui-preview.surge.sh

@yaacov yaacov requested review from rszwajko, mturley and sjd78 August 24, 2022 12:31
@@ -203,7 +203,7 @@ export const checkIfResourceExists = async (
export const useClientInstance = (): KubeClient.ClusterClient => {
const { currentUser } = useNetworkContext();
const user = {
access_token: currentUser?.access_token || '',
access_token: currentUser?.access_token || window['_meta']?.clusterBearerToken || '',
Copy link
Collaborator

@mturley mturley Aug 24, 2022

Choose a reason for hiding this comment

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

Instead of directly referencing window['_meta'], the properties from the meta.json file should be added to the IMetaVars type here and fallback defaults for each should be added to the exported META variable here. Then this file can import { META } from '@app/common/constants'; and you can use META.clusterBearerToken here.

That way we get strong types for these (useful for editor autocomplete and avoiding typos) and don't need the ?. and the || ''.

Copy link
Collaborator

@mturley mturley Aug 24, 2022

Choose a reason for hiding this comment

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

Actually, alternatively you could use an environment variable for this. the meta.json file is generated by the operator and generally used for things that also apply to prod but can be overridden in dev. If it's an environment variable it could be overridden on the command line if necessary. I honestly can't remember exactly why we did both (@ibolton336 may remember), and the distinction has probably been broken, but just so you're aware of both:

To define a new environment variable you add it to the IEnvVars type here, use import { ENV } from '@app/common/constants'; (defined here), and make sure to add it to the list on the server here so it gets bundled up and sent to the client.

Both these META and ENV mechanisms could probably be combined and/or replaced, they were hacks that stuck around... but the strong types are useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 👍, I like the environment variable approach better then the meta file for a token parameter,
changed the code to use the ENV, and added the type as recomended/

Copy link
Member Author

Choose a reason for hiding this comment

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

do we document env vars someware ?

Copy link
Member Author

Choose a reason for hiding this comment

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

added documentation to README.md file

@yaacov yaacov force-pushed the allow-to-use-static-token branch 3 times, most recently from 177ec6b to 307c04b Compare August 25, 2022 08:39
@yaacov yaacov requested a review from mturley August 25, 2022 08:41
@yaacov yaacov force-pushed the allow-to-use-static-token branch 2 times, most recently from 2a37d23 to 7f3c09c Compare August 25, 2022 08:53
Copy link
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks

@mturley
Copy link
Collaborator

mturley commented Aug 25, 2022

Agh sorry, looks like I caused a merge conflict by merging #984

@yaacov yaacov force-pushed the allow-to-use-static-token branch from 7f3c09c to 37f741a Compare August 28, 2022 07:28
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yaacov yaacov requested a review from mturley August 28, 2022 07:32
@yaacov
Copy link
Member Author

yaacov commented Aug 28, 2022

@mturley np :-), rebased

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

Successfully merging this pull request may close these issues.

4 participants