-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update "make clean" to remove Redash dev Docker images #6847
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6847 +/- ##
=======================================
Coverage 63.82% 63.82%
=======================================
Files 161 161
Lines 13060 13060
Branches 1803 1803
=======================================
Hits 8335 8335
Misses 4425 4425
Partials 300 300 |
3e5dde6
to
2b1ae8a
Compare
Makefile
Outdated
docker compose --project-name cypress down | ||
docker compose rm --stop --force | ||
docker compose --project-name cypress rm --stop --force | ||
docker compose rm |
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.
This is looking more complete, but I think we do not need docker compose rm
since this is already done by docker compose rm --stop --force
.
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.
Heh Heh Heh. Ok, I'll take another pass at it... 😉
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.
Looking at it now, yeah you're right. Not sure how I doubled that up. Must have been really tired or something. 😄
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.
k, I've just pushed an updated commit that removes the doubling up, and verified it's still all working as intended. It cleanly removes the running redash and cypress containers, and removes the corresponding image files as well.
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.
Hmmm, I've just rebased this on master
too while I'm at it, so its easier to merge.
73e0e04
to
3ecbf70
Compare
Makefile
Outdated
docker image prune --force | ||
docker volume prune --force | ||
|
||
clean-all: |
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.
You'd want to add this target to the .PHONY
section... most of the rules in this makefile do not create a target file.
Then add it as a dependency
clean-all: clean
Or maybe not? I wouldn't mind typing make clean; make clean-all
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.
Thanks @eradman. I'm clearly still coming up to speed with this stuff again. 😇
Looking at that bit now.
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.
Personally I'd rather keep the clean-all
target incorporating the clean
one. But I'm not even sure if we really need it. 😄
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.
k, updated. clean-all
added to the .PHONY
list, and clean
added as a dependency of clean-all
so it doesn't need embedding as make clean
.
In testing, make clean
is removing both the redash and cypress containers + dev images, with make clean-all
removing them plus the associated images (redis, pgautoupgrade, last redash release).
Also added a "make clean-all" target to remove the related containers
3ecbf70
to
ae89066
Compare
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'm good with this; docker does not always track dependencies correctly, so it is necessary sometimes to force a rebuild.
Thanks heaps @eradman, merging it now. 😄 |
What type of PR is this?
Description
At present, when a Redash developer wants to remove the locally built Redash Docker images, they need to do it manually.
This PR extends the existing
make clean
command to remove those images as well.For example, here is the list of local Redash docker images after building and testing Redash:
With this PR, the
make clean
command will remove the Redash ones:With the result:
Also added a
make clean-all
command, which also removes the above redis, pgautoupgrade, and maildev images too. That one's probably only rarely useful however. 😄How is this tested?
This was tested by building the Redash images (
make compose_build
,yarn cypress build
), then running the updatedmake clean
command and verifying the result.