Skip to content
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

Improve developer experience #146

Merged
merged 7 commits into from
Nov 6, 2024

Conversation

lukasturcani
Copy link
Contributor

@lukasturcani lukasturcani commented Oct 12, 2024

This PR aims to make the experience of developing a new feature for NOMAD slightly smoother.

The general workflow of a developer when making a new branch is:

  1. Create a new clean branch
  2. Run cp -r env-example env
  3. Set values for ADMIN_PASSWORD and JWT_SECRET entries in env/backend.env
  4. Set values for ADMIN_PASSWORD and JWT_SECRET entries in env/backend-test.env
  5. comment out the client service in docker-compose.yaml
  6. Run docker compose up to create a new developer instance

I don't think steps 2, 3, 4 and 5 should be necessary.

To remove the need for this I've made the following changes:

  • created a new directory called envs, this holds the old env-example directory as well as a new dev directory. The dev directory has preconfigured values for ADMIN_PASSWORD AND JWT_SECRET suitable for development
  • The docker-compose.yaml file now uses envs/dev/backend.env and frontend.env
  • I've added the client profile to the client service in docker-compose.yaml This means that if the developer is using the client they can run docker compose --profile client up which will enable this service but it will be off by default

There is also a change I've made to nomad-rest-api/Dockerfile. Currently, when a new dependency is added, it is kind of hard to update the dependencies in the dev containers. This is because the dependencies are placed into a separate volume called /app/node_modules. If I change the package.json and rebuild the image, it does not update the content in the /app/node_modules volume, since building an image happens before a volume is attached. Once the volume is attached to the container, it hides the new content of /app/node_modules and uses the old deps.

So the only way to update my deps for the docker container is to delete the volume, which is kind of inconvenient, not only because it requires an additional command but also because the /app/node_modules volume is anonymous and therefore hard to identify.

However, by changing the Docker to command: npm install && ... every time docker compose up is run again, and the container is entered after the volume is attached, npm install will be run and the volume will have its contents updated.

TLDR: The only thing now necessary to update deps in the dev container should be re-running docker compose up

@tomlebl
Copy link
Contributor

tomlebl commented Oct 28, 2024

@lukasturcani I don't think that the change in the Dockerfile is necessary. If there is a change in dependencies then you just need to start the project with docker-compose up -d --build and that's it. I have just tried and it works. This information is missing in documentation and read.me file. Something I should have done a while ago. My bad.
I would rather keep it as it is now, more aligned with the production Dockerfiles.

Using profile for the client is a good idea. I quite like that.
I am fine with the proposed changes in ENV files.

@tomlebl tomlebl changed the base branch from main to improve-devx October 28, 2024 17:13
@tomlebl tomlebl merged commit a641cf2 into nomad-nmr:improve-devx Nov 6, 2024
2 checks passed
tomlebl added a commit that referenced this pull request Nov 6, 2024
* Improve developer experience (#146)

* Improve devX

* Fix tests

* Add values

* Update README.md

* sockets url fix

* Undo dockerfile changes

---------

Co-authored-by: Tomas Lebl <tl12@st-andrews.ac.uk>

* gitignore & read,me update

---------

Co-authored-by: Lukas Turcani <lukasturcani93@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants