-
Notifications
You must be signed in to change notification settings - Fork 354
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(docker): Build Docker images once, scan them for vulnerabilities during CI #3997
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3997 +/- ##
==========================================
+ Coverage 96.22% 96.42% +0.19%
==========================================
Files 1142 1146 +4
Lines 36760 37386 +626
==========================================
+ Hits 35371 36048 +677
+ Misses 1389 1338 -51 ☔ View full report in Codecov by Sentry. |
ce4978f
to
9eac5ac
Compare
9eac5ac
to
dbc584f
Compare
dbc584f
to
d35241c
Compare
ba6f8f0
to
e8ba0b1
Compare
51f9300
to
768461a
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.
Added some minor comments but overall looks great 👌
|
||
inputs: | ||
api-image: | ||
description: API image to use |
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 think it would be good to provide more information here. What format is this expected to be in?
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.
Example value added.
type: string | ||
description: Path to the Dockerfile | ||
required: true | ||
slug: |
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.
slug: | |
image-slug: |
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've also never heard the term slug
used for an image. Why not use e.g. image-name
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.
Renamed to image-name
.
Yes, this doesn't make an awful lot of sense. I think we can just consolidate this to only test via docker since this is the product that we provide to our customers. |
- Remove local e2e jobs in favour of dockerised - Add `make test` target for frontend
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
This does a few things:
During the making of this PR, I've discovered a number of discussion points and action listed below.
Things to consider / TODOs
̶f̶l̶a̶g̶s̶m̶i̶t̶h̶/̶f̶l̶a̶g̶s̶m̶i̶t̶h̶
̶.̶ ̶H̶o̶w̶e̶v̶e̶r̶,̶ ̶w̶h̶a̶t̶ ̶i̶s̶ ̶n̶o̶w̶ ̶c̶a̶l̶l̶e̶d̶ ̶"̶E̶2̶E̶ ̶U̶n̶i̶f̶i̶e̶d̶"̶ ̶a̶c̶t̶u̶a̶l̶l̶y̶ ̶r̶u̶n̶s̶ ̶s̶e̶p̶a̶r̶a̶t̶e̶ ̶A̶P̶I̶ ̶a̶n̶d̶ ̶f̶r̶o̶n̶t̶e̶n̶d̶ ̶s̶e̶r̶v̶i̶c̶e̶s̶ ̶s̶t̶i̶l̶l̶.̶ ̶C̶u̶r̶r̶e̶n̶t̶l̶y̶,̶ ̶t̶h̶e̶ ̶o̶n̶l̶y̶ ̶d̶i̶f̶f̶e̶r̶e̶n̶c̶e̶ ̶b̶e̶t̶w̶e̶e̶n̶ ̶'̶l̶o̶c̶a̶l̶'̶ ̶a̶n̶d̶ ̶'̶u̶n̶i̶f̶i̶e̶d̶'̶ ̶i̶s̶ ̶w̶h̶e̶t̶h̶e̶r̶ ̶f̶r̶o̶n̶t̶e̶n̶d̶ ̶i̶s̶ ̶b̶e̶i̶n̶g̶ ̶r̶u̶n̶ ̶i̶n̶ ̶D̶o̶c̶k̶e̶r̶.̶ ̶@̶m̶a̶t̶t̶h̶e̶w̶e̶l̶w̶e̶l̶l̶ ̶@̶n̶o̶v̶a̶k̶z̶a̶b̶a̶l̶l̶a̶ ̶W̶e̶ ̶n̶e̶e̶d̶ ̶a̶ ̶d̶e̶c̶i̶s̶i̶o̶n̶ ̶o̶n̶ ̶t̶h̶i̶s̶.̶ This is now resolved in favour of Docker-based E2E runs.frontend/Dockerfile.e2e
build mostly duplicatesfrontend/Dockerfile
one. I'd like to try a multi-stage Docker build for E2E, i.e. add aFROM ghcr.io/flagsmith/flagsmith-frontend:${FRONTEND_VERSION} as frontend
stage and pull whatever we need on top of the base E2E image. I couldn't tackle this on my own so tagging @kyle-ssg for advice on this.How did you test this code?
This is a CI change.