-
Notifications
You must be signed in to change notification settings - Fork 6
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
CI: Implement a publish action to push Docker images #43
base: master
Are you sure you want to change the base?
Conversation
02e4144
to
0e7353f
Compare
Thanks @HarshCasper - can you add a little extra context explaining the pipeline? Where will it be published, how often is it run etc. |
Signed-off-by: HarshCasper <erbeusgriffincasper@gmail.com>
Signed-off-by: HarshCasper <erbeusgriffincasper@gmail.com>
Signed-off-by: HarshCasper <erbeusgriffincasper@gmail.com>
Signed-off-by: HarshCasper <erbeusgriffincasper@gmail.com>
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.
Thank you for the PR, Harsh! It looks good except for some minor issues.
name: Docker CI/CD | ||
|
||
on: | ||
schedule: |
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 don't see the need of publishing a new image every day. Can you elaborate on your intention behind 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.
It was something that we have done with upstream images to ensure that we get to know if something breaks (like on FLINT and FLINT.Example). Hence we add this additional step to build and push the images on a cron schedule. Let me know if we can remove this in case of Taswira.
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.
Got it. While that sounds like a good step, I don't think it's required for taswira because there's nothing in it that could break overnight. I think, things will only break after a change. Do you agree?
@@ -15,3 +15,5 @@ dependencies: | |||
- pylint>=2.5.2 | |||
- dash==1.13.3 | |||
- dash-leaflet==0.0.19 | |||
- markupsafe<2.1 |
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 these two new dependencies? Where are they used?
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.
Without these dependencies, the build fails. Reference: https://mojaglobal.slack.com/archives/C01113X44FR/p1646205676283029
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.
Ah, I see, but not sure why this issue didn't occur in my machine.
Anyway, can you pin jinja2
to an absolute version number?
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@v2 | ||
- name: Lint using pylint |
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 step that lints the code using pylint
is missing in the new workflow.
I had added that step to ensure code quality. It'd be great if we can add it to the new workflow 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.
I will add that!
Fixes #42