-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add Makefile command for building code #201
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #201 +/- ##
========================================
Coverage 88.69% 88.69%
========================================
Files 6 6
Lines 398 398
========================================
Hits 353 353
Misses 45 45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
just one small question but it LGTM
Makefile
Outdated
# Build the docker | ||
build: | ||
build-app: | ||
docker build . -t pyronear/pyro-engine:python3.8.1-slim |
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.
why do you build two images here ?
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.
hum build-app is building one image, with the app contained in src/ and build-lib is building the library locally.
Maybe build-image and build-lib would be a better wording ?
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.
Sorry, maybe it's just me, but I don't understand why we need these two lines, we can't just keep :
docker build . -t pyronear/pyro-engine:latest
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.
done @MateoLostanlen
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.
All good thanks
I was a bit lost when I tried to build the code because of the separation between the lib and the app so I have added some Makefile commands in order to better reflect this architecture and I removed one Makefile file and the requirements.txt for simplification purpose