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

Finalize local dev env setup #4

Merged
merged 14 commits into from
Jul 23, 2024
Merged

Finalize local dev env setup #4

merged 14 commits into from
Jul 23, 2024

Conversation

ronaldo-macapobre
Copy link

@ronaldo-macapobre ronaldo-macapobre commented Jul 19, 2024

Pull Request for JIRA Ticket: 10

Issue ticket number and link

https://jag.gov.bc.ca/jira/browse/JASPER-10

Description

  • Enable hot reloading for frontend code
  • Add more info to README.md for local development.
  • Changes made to the vue.config.js is necessary to avoid proxy error issues and does not affect the behavior of the old workflow where web artifact is used.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • Tested that hot reloading works in frontend code.
  • Tested that old workflow still works.

Test Configuration:
If applicable

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Documentation References

N/A

Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

Awesome job @ronaldo-macapobre!

A few recommendations.

  1. Add the launch.json changes to the dev container configuration so it's always there for everyone by default.
  2. Have the build always build all project containers. This avoids having to make specific updates to the script.
  3. Update the the ./manage script to add a debug command that would start the set of containers for hot reloading and debugging, where the start command would start the compiled runtime set of containers. This avoids having to edit the ./manage script to access the different features.

@WadeBarnes
Copy link
Member

WadeBarnes commented Jul 19, 2024

Regarding the line ending issues. You could add some additional lines to cover affected files to this file to ensure the line endings are always set as Lf; https://github.com/bcgov/jasper/blob/master/.gitattributes

Copy link

@amlanc1 amlanc1 left a comment

Choose a reason for hiding this comment

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

lgtm

@WadeBarnes
Copy link
Member

One item that might be worth noting in the documentation. The dev container will fail to build if you are connected to the BC Government VPN when building/rebuilding.

- updated ./manage to include start-debug and build-debug
- updated README.md content
@ronaldo-macapobre
Copy link
Author

ronaldo-macapobre commented Jul 20, 2024

Awesome job @ronaldo-macapobre!

A few recommendations.

  1. Add the launch.json changes to the dev container configuration so it's always there for everyone by default.
  2. Have the build always build all project containers. This avoids having to make specific updates to the script.
  3. Update the the ./manage script to add a debug command that would start the set of containers for hot reloading and debugging, where the start command would start the compiled runtime set of containers. This avoids having to edit the ./manage script to access the different features.

@WadeBarnes I tried suggestion 3. to build everything (web web-dev api db) but I encountered an error saying 0.0.0.0:8080 is already used when switching from ./manage debug and ./manage start even if I remove all containers, images and volumes. I tried simplifying the code so I added build-debug and start-debug. Let me know what you think.

- Simplify build
- Simplify commands
- Restore use of getStartupParams

Signed-off-by: Wade Barnes <[email protected]>
- Relabel from `scv` and `scjscv` to `jasper`.

Signed-off-by: Wade Barnes <[email protected]>
- to reflect updates to the `./manage` script.

Signed-off-by: Wade Barnes <[email protected]>
Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

Hey @ronaldo-macapobre, Looking good!

I've added a few commits:

  1. To further cleanup the ./manage script and restore some functionality that was lost in the shuffle.
  2. To relabel the images and routes from scv to jasper.
  3. To update the development environment documentation based on the above changes.

Let me know what you think. If you're happy with the updates we can merge the PR.

Ronaldo Macapobre and others added 4 commits July 22, 2024 16:51
- Allows the dev container to share the host's docker instance, allowing the host to see any containers launched in the dev container.

Signed-off-by: Wade Barnes <[email protected]>
- changed host.docker.interal to container name
Ronaldo Macapobre and others added 2 commits July 22, 2024 17:40
- We missed switching a couple local folder mounts over to using `LOCAL_WORKSPACE_FOLDER`.

Signed-off-by: Wade Barnes <[email protected]>
Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

I think that's got it. Great work @ronaldo-macapobre

@ronaldo-macapobre ronaldo-macapobre merged commit 7ad2759 into master Jul 23, 2024
2 checks passed
@ronaldo-macapobre ronaldo-macapobre deleted the dev-env-setup branch July 23, 2024 20:20
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.

3 participants