-
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
trigger preview image build on tag #6933
trigger preview image build on tag #6933
Conversation
* reuse built frontend in ci, merge compose files * pr comments * added make create_db alias to create_database * fixed lint --------- Co-authored-by: Andrii Chubatiuk <wachy@Andriis-MBP-2.lan>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6933 +/- ##
=======================================
Coverage 63.76% 63.76%
=======================================
Files 161 161
Lines 13085 13085
Branches 1812 1812
=======================================
Hits 8344 8344
Misses 4438 4438
Partials 303 303 |
The format for a tagged image previous should be |
this workflow creates a tag |
bin/docker-entrypoint
Outdated
--max-requests $MAX_REQUESTS \ | ||
--max-requests-jitter $MAX_REQUESTS_JITTER \ | ||
--timeout $TIMEOUT \ | ||
--reload |
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.
@eradman added hotreload for gunicorn
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 should be in a separate PR
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.
Yeah, it does seem unrelated to the purpose of this PR. 😄
@AndrewChubatiuk Please move the unrelated stuff (like this) to a separate PR. More than one if needed.
Mixing stuff together just slows down the reviewing. 😦
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.
@eradman @justinclift moved this change to #6940
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 @AndrewChubatiuk. 😄
a8d2691
to
d2ae4fd
Compare
FULL_VERSION=${VERSION} | ||
else | ||
FULL_VERSION=${VERSION}-b${GITHUB_RUN_ID}.${GITHUB_RUN_NUMBER} | ||
fi |
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.
@eradman Reading over these lines here, doesn't the FULL_VERSION
variable end up how it used to be for the monthly snapshots?
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.
Previously the trigger condition was based on a single condition
name: Preview Image
on:
push:
tags:
- '*-dev'
This made the entire workflow easy to reason about. I don't know what will happen when the workflow is run; needs to be tested in a private repository.
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.
it's needed to:
- prevent building image with code, that doesn't pass tests in master
- spend less time on ARM64 images build, which are only possible to build on QEMU in github actions
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.
on QEMU in github actions
That raises an interesting point. GitHub Actions added M1 based macOS runners a few months ago. Wonder if means we could build ARM64 images natively using those?
Might be too much hassle to try and figure that out though. Not sure. 😄
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.
it doesn't solve a problem, still emulation is required
d2ae4fd
to
a1ad984
Compare
If we are going to push a new docker image for each commit, I think we should create two different workflows:
This does results in duplicate configuration, but I find workflows very tedious to test properly, so the conditions need to be as simple as possible. @justinclift what do you think? |
I've tested these changes in my fork. it works as expected |
if [[ "$BRANCH" =~ ^[0-9]+\.[0-9]+\.[0-9]+\-dev ]]; then | ||
FULL_VERSION=${VERSION} | ||
else | ||
FULL_VERSION=${VERSION}-b${GITHUB_RUN_ID}.${GITHUB_RUN_NUMBER} |
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.
Is this what we want? This used to be the short SHA of the commit.
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.
it's what it used to be
you can check images, that i've published during testing in a fork
https://hub.docker.com/r/achubatiuk/preview/tags
Personally, I think we should have some kind of docker image that always has the latest and greatest development tip from the master branch. That has the use of pointing people at it to verify a fix they reported (or need for some reason), and it's also useful for us to try out. That being said, we also need a more stable image we can recommend to people, as we work towards an official, proper release. The For that, we'll probably need to branch off of master at some near future point, test the heck out of things at that point and fix any bugs we find (applying the fixes both in that branch and in the master branch). After a few iterations of fixes (hopefully not many), it should be a pretty good image for making official. Note that I don't think we need a brand new unique docker image for every commit to master. That's going to be horrible for people to try and figure out what image to grab, and horrible for whoever's looking after the Docker Hub repository (@arikfr presently I think). |
if user wants to have a certain image, which corresponds to a git tag or just a latest image, he has such an option, but at the same time we need to allow people to rely on build they can consider stable themselves. Either monthly images, which are triggered automatically without stable changes with random content inside and without separate release branches for stability fixes or latest images is not a good option for those, who have infrastructure, which scales up and down often enough, especially taking in account amount of developers, who are able to fix Redash issues. It forces users either build images on their own if image, that was build from master or tag are broken or wait till it's fixed and hope, that it will not become broken again soon |
Yeah. I guess I'm thinking we need to think through how to make a stable branch in the near future, which we then test the living heck out of and eventually call it our next release. 😄 |
What type of PR is this?
trigger pipelines on tags
Description
How is this tested?
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)