-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Added env flag for github oauth option #644
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @BiswaViraj this looks great! I have a small suggestion I've added in the last comment to use a unified static variable to indicate we are running inside self-hosted docker. We could use this for other uses later. Let me know what you think :)
docker/docker-compose.yml
Outdated
@@ -69,6 +69,7 @@ services: | |||
REACT_APP_API_URL: ${API_ROOT_URL} | |||
REACT_APP_ENVIRONMENT: ${NODE_ENV} | |||
REACT_APP_WIDGET_EMBED_PATH: ${WIDGET_EMBED_PATH} | |||
REACT_APP_GITHUB_AUTH_OPTION: ${REACT_APP_GITHUB_AUTH_OPTION} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REACT_APP_GITHUB_AUTH_OPTION: ${REACT_APP_GITHUB_AUTH_OPTION} | |
REACT_APP_DOCKER_HOSTED_ENV: true |
Hi @BiswaViraj a small suggestion, let me know what you think.
The reasoning would be that we might later add some more conditional functionality based on whether the user is in a docker mode or not, so we can use a more general static name that cannot be changed on the env. (We can even consider adding this to the docker file itself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes sense to update it to a generic one.
I'll update the PR
docker/.env.example
Outdated
# Github Auth Option | ||
REACT_APP_GITHUB_AUTH_OPTION=hide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going with my suggestion we can even remove this one.
apps/web/src/config/index.ts
Outdated
@@ -23,3 +23,5 @@ export const ENV = process.env.REACT_APP_ENVIRONMENT; | |||
export const APP_ID = process.env.REACT_APP_NOVU_APP_ID; | |||
|
|||
export const WIDGET_EMEBED_PATH = process.env.REACT_APP_WIDGET_EMBED_PATH || 'http://localhost:4701/embed.umd.min.js'; | |||
|
|||
export const GITHUB_AUTH_OPTION = process.env.REACT_APP_GITHUB_AUTH_OPTION === 'show'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const GITHUB_AUTH_OPTION = process.env.REACT_APP_GITHUB_AUTH_OPTION === 'show'; | |
export const GITHUB_AUTH_OPTION = !!process.env. REACT_APP_DOCKER_HOSTED_ENV; |
apps/web/.env
Outdated
@@ -2,3 +2,4 @@ SKIP_PREFLIGHT_CHECK=true | |||
REACT_APP_API_URL=api_url | |||
REACT_APP_WIDGET_SDK_PATH=sdk_path | |||
REACT_APP_ENVIRONMENT=dev | |||
REACT_APP_GITHUB_AUTH_OPTION=show |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one could be removed aswell.
Proper Error Message when used without NovuProvider
@scopsy , I've updated the changes, please check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect @BiswaViraj !
Fixes [NV-469] - Hide GitHub OAuth when running inside Docker #626
Github OAuth button is being displayed on self-host server
Removed the Github OAuth button if it is self-hosted to avoid confusion.
Added
REACT_APP_GITHUB_AUTH_OPTION
to the env to control the display of the Github OAuth button