-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fixes to build process #1530
Fixes to build process #1530
Conversation
Deploying with
|
Latest commit: |
bfd922a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://29d42a19.taqueria.pages.dev |
Branch Preview URL: | https://improvements-to-build-and-te.taqueria.pages.dev |
|
npm install | ||
npm run bootstrap | ||
echo "** Checking Dependencies" | ||
docker ps |
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.
What is this for?
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.
Docker is used at the very last steps of build-all
. So if the engine is not running, the user will be informed only after building everything and failing at the last step. With this change, they will be notified right after the run command build-all
.
@@ -6,7 +6,7 @@ | |||
"main": "index.js", | |||
"scripts": { | |||
"build-package": "npx tsc -noEmit && npx esbuild --platform=node --bundle index.ts --outdir=.", | |||
"build-docker": "TAQ_VERSION=`taq --version` BUILD=`taq --build`; docker build . -t \"ghcr.io/ecadlabs/taqueria-flextesa:${TAQ_VERSION}-${BUILD}\"", | |||
"build-docker": "TAQ_VERSION=`../taq --version` BUILD=`../taq --build`; docker build . -t \"ghcr.io/ecadlabs/taqueria-flextesa:${TAQ_VERSION}-${BUILD}\"", |
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.
Can we source these from git
instead taq
?
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.
We can. I'm not sure which one is better. Or even if there is a real difference for us.
What's your opinion?
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.
I mean, I felt that this was the smaller change that fixed the problem we already have, so I went with this.
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 can use git it'd be more desirable
The docker image has been published and is avaiable here. Look for the image with the tag |
The docker image has been published and is avaiable here. Look for the image with the tag |
The docker image has been published and is avaiable here. Look for the image with the tag |
a7c8e26
to
8269ddd
Compare
The docker image has been published and is avaiable here. Look for the image with the tag |
The docker image has been published and is avaiable here. Look for the image with the tag |
bf6cc67
to
ab5cd92
Compare
The docker image has been published and is avaiable here. Look for the image with the tag |
ab5cd92
to
a6c9205
Compare
The docker image has been published and is avaiable here. Look for the image with the tag |
The docker image has been published and is avaiable here. Look for the image with the tag |
The docker image has been published and is avaiable here. Look for the image with the tag |
The docker image has been published and is avaiable here. Look for the image with the tag |
9d39fa4
to
ad97174
Compare
The docker image has been published and is avaiable here. Look for the image with the tag |
The docker image has been published and is avaiable here. Look for the image with the tag |
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.
LGTM! Just left one comment but if it's not easy to do, it's no problem.
@@ -6,7 +6,7 @@ | |||
"main": "index.js", | |||
"scripts": { | |||
"build-package": "npx tsc -noEmit && npx esbuild --platform=node --bundle index.ts --outdir=.", | |||
"build-docker": "TAQ_VERSION=`taq --version` BUILD=`taq --build`; docker build . -t \"ghcr.io/ecadlabs/taqueria-flextesa:${TAQ_VERSION}-${BUILD}\"", | |||
"build-docker": "TAQ_VERSION=`../taq --version` BUILD=`../taq --build`; docker build . -t \"ghcr.io/ecadlabs/taqueria-flextesa:${TAQ_VERSION}-${BUILD}\"", |
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 can use git it'd be more desirable
The docker image has been published and is avaiable here. Look for the image with the tag |
🌮 Taqueria PR
🪁 Description
Explain it to me like I'm 5! Keep this short and descriptive
🪂 Pre-Merge Checklist (Definition of Done)
🚦 Required to merge:
🛩️ Summary of Changes
Please include a summary of the changes to the codebase. Please also include relevant motivation and context for the change
(e.g. what was the existing behaviour, why did it break, etc.)
🎢 Test Plan
Please describe the testing strategy and plan for this PR. Keep this lightweight and anticipate any testing challenges
🛸 Type of Change