Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Multiple environments #88

Merged
merged 19 commits into from
Aug 31, 2020
Merged

Conversation

sladyn98
Copy link
Contributor

@sladyn98 sladyn98 commented Jul 24, 2020

This PR tries to use multiple environments so that the host:name can be modified when the docker compose is run and the when the local environment is run.
For now I am using a library called as env-cmd. I created a .env.docker in which the API_URL connects to the required app server when the docker compose is used.In order to insert the new environment file I defined a new script called start:socker inside of scripts which runs using npm run start:docker to insert the new environment file. When the docker compose is not used it defaults to the normal .env file.
When you run this pr using maven and npm it runs perfectly but fails with docker-compose

Update: This is now working. Can someone test it on their machine

@sladyn98 sladyn98 requested a review from a team as a code owner July 24, 2020 06:22
@kwhetstone
Copy link
Contributor

@sladyn98 I think at this point. in order to get this PR merged, you have to rebase and determine if there are any fixes that need to be incorporated. This is sort of a large change. so it makes sense to essentially start again with this PR

@sladyn98
Copy link
Contributor Author

sladyn98 commented Aug 4, 2020

@kwhetstone Fixed conflicts and it works.

@sladyn98
Copy link
Contributor Author

sladyn98 commented Aug 4, 2020

a) Check if the environment variables are set during startup. Maybe set a default. LOG message as to what the default is.
b) Update the readme to state the configuration option

@sladyn98
Copy link
Contributor Author

sladyn98 commented Aug 6, 2020

Need to make the readme up to date with all of the build instructions

@martinda
Copy link
Contributor

martinda commented Aug 9, 2020

Please rebase this branch on the latest master. On your local computer, the operations would be to git checkout multiple-environments; git rebase master

Copy link
Contributor

@martinda martinda left a comment

Choose a reason for hiding this comment

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

I added to the design document the requirements for the configurable URL where configurations are stored.

What is needed in this PR:

  • details in the README of how to change the values of the REACT_APP_API_URL (which files to change), for each use case (docker, non-docker)
  • same details for how to change the REACT_APP_GITHUB_COMMUNITY_URL (which file to change)

frontend/.env.docker Show resolved Hide resolved
frontend/.env.docker Show resolved Hide resolved
Copy link
Contributor

@martinda martinda left a comment

Choose a reason for hiding this comment

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

I am also getting a new issue, see #137

README.md Outdated Show resolved Hide resolved
frontend/.env Outdated Show resolved Hide resolved
Copy link
Contributor

@martinda martinda left a comment

Choose a reason for hiding this comment

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

Please rebase this PR.

@sladyn98 sladyn98 requested a review from martinda August 20, 2020 06:04
@sladyn98
Copy link
Contributor Author

@kwhetstone @martinda I tested this
a) Using docker compose
b) Using maven and npm
Tested the generation end to end and it works fine so this is ready to be reviewed.

Copy link
Contributor

@martinda martinda left a comment

Choose a reason for hiding this comment

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

Getting the 404 when changing the community link is the biggest unsolved problem.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
frontend/.env.docker Show resolved Hide resolved
@martinda
Copy link
Contributor

Now I know why these port numbers have been confusing me. Without docker, the port is localhost:3001, but with docker, the port becomes localhost:3000. A table in the README file would make this obvious.

@sladyn98 sladyn98 requested a review from martinda August 23, 2020 11:09
@halkeye
Copy link
Member

halkeye commented Aug 23, 2020

Can you make sure REACT_APP_API_URL is set to / in Dockerfile.infra? ENV REACT_APP_API_URL=/

FYI env variables are replaced at compile time, so its not something that can be configured at run time for different servers.

Longer term it might be better for spring boot to handle configuration so it can be changed without having to rebuild the entire javascript app

<script src="/path/to/springbootapp/config.js"></script>

window.Config = window.Config || {}
window.Config.API_URL = "abcd123"

@sladyn98
Copy link
Contributor Author

Can you make sure REACT_APP_API_URL is set to / in Dockerfile.infra? ENV REACT_APP_API_URL=/

I am not sure how am I to do this? Since this env file is controlled by the .env file

@halkeye
Copy link
Member

halkeye commented Aug 23, 2020

ENV REACT_APP_API_URL=/

Sorry it's space not equals

ENV REACT_APP_API_URL /

@sladyn98
Copy link
Contributor Author

ENV REACT_APP_API_URL=/

Sorry it's space not equals

ENV REACT_APP_API_URL /

Do I add it to the dockerfile infra?

@halkeye
Copy link
Member

halkeye commented Aug 23, 2020

Yep, Dockerfile.infra, in the last FROM block

maybe around the EXPOSE https://github.com/jenkinsci/custom-distribution-service/blob/master/Dockerfile.infra#L36

@halkeye
Copy link
Member

halkeye commented Aug 23, 2020

Oh wait. It needs to be set before npm build in the front-end section in the middle

@sladyn98
Copy link
Contributor Author

@halkeye Does this look alright ?

@halkeye
Copy link
Member

halkeye commented Aug 24, 2020

looks right. does it build?

docker build -t somename -f Dockerfile.infra .

@sladyn98
Copy link
Contributor Author

@halkeye Yeah it builds

@sladyn98
Copy link
Contributor Author

@martinda @kwhetstone Can we merge this in as is, and fix the issues later if there are any, since this was a lot of work and it has been open for more than a month now, the more we delay i guess the more we would have to keep fixing this.

@sladyn98 sladyn98 merged commit f8ea520 into jenkinsci:master Aug 31, 2020
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.

None yet

4 participants