SCV-459 Migration to Emerald (work in progress) #287
Conversation
There was a problem hiding this comment.
Please see my comments on this PR; bcgov#9 (review). I have the same concerns with the changes made here.
I'm very concerned that the build processes used in the manage scripts and in the GitHub workflows are diverging. This leads to inconsistencies between the code and images running on the developer's machine, compared to what's built and deployed to the deployment environments. This WILL lead to confusion in the future and the "works on my machine" issues that we're trying to avoid by using containers in the first place. To put is simply, the container builds should be consistent so what's running on a developers machine is basically identical to what's being deployed.
| - name: Checkout Repo | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: SCV-415 # Specifies the branch to checkout |
There was a problem hiding this comment.
This hard coded branch does not exist in the main repo.
There was a problem hiding this comment.
This is a temporary solution that allows me to test the Pre-download functionality that only exists in my own repo at the moment. We can wait to merge until this is finalized.
api/Dockerfile
Outdated
There was a problem hiding this comment.
Dockerfiles should live in the docker folder.
| username: ${{ secrets.ARTIFACTORY_USERNAME }} | ||
| password: ${{ secrets.ARTIFACTORY_PASSWORD }} | ||
|
|
||
| - name: Checkout Schema Spy Repo |
There was a problem hiding this comment.
| - name: Checkout Schema Spy Repo | |
| - name: Checkout Backup-Container Repo |
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: BCDevOps/backup-container.git | ||
| ref: master |
There was a problem hiding this comment.
The ref should be using a tagged version of the backup-container rather than the master branch.
| - name: Checkout Repo | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: SCV-415 # Specifies the branch to checkout |
There was a problem hiding this comment.
This hard coded branch does not exist in the main repo.
There was a problem hiding this comment.
This is a temporary solution that allows me to test the Pre-download functionality that only exists in my own repo at the moment. We can wait to merge until this is finalized.
web/Dockerfile.node
Outdated
| @@ -0,0 +1,10 @@ | |||
| FROM centos/nodejs-10-centos7:10 | |||
There was a problem hiding this comment.
This should be using node 12 and it should not be hard coded.
web/Dockerfile.node
Outdated
There was a problem hiding this comment.
Dockerfiles should live in the docker folder.
I'm adding this pull request at Wade's request to improve the visibility of my work.