-
Notifications
You must be signed in to change notification settings - Fork 832
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
#1280 Fix Dockerfiles for .NET 6 SDK #1281
Conversation
imagePullPolicy: Always | ||
image: <REGISTRY_NAME>/yarp-controller:<TAG> | ||
imagePullPolicy: IfNotPresent | ||
image: yarp/yarp-controller |
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 changed this so that I don't have to keep modding the file locally to use 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.
Sadly we can't check in a reference to your image here for security reasons
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 not publishing any images. This is simply a placeholder name for the local build process. The registry name <REGISTRY_NAME>
is invalid, so it needs to be modified when working locally.
yarp
could be replaced by anything really: local
, donotuse
, etc.
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 trust you wouldn't, but presumably whoever owns the yarp
name could potentially push an image to the repository tomorrow.
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'll revert that line until yarp publishes official image.
README.md | ||
**/ingress.yaml |
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.
Prevents unnecessary docker rebuilds when modifying the kube manifests
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 for fixing these up!
COPY ["src/", "src/"] | ||
COPY ["Directory.Build.*", ""] | ||
COPY ["global.json", ""] | ||
COPY ["NuGet.config", ""] |
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.
Would it be simpler to copy the entire source folder here? We will only copy the published artifacts regardless
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 was attempting to follow this multi-stage approach https://docs.microsoft.com/en-us/visualstudio/containers/container-build?view=vs-2019&WT.mc_id=visualstudio_containers_aka_containerfastmode
But I now realise the Dockerfile doesn't include a nuget package layer. I'll push an update that fixes that.
imagePullPolicy: Always | ||
image: <REGISTRY_NAME>/yarp-controller:<TAG> | ||
imagePullPolicy: IfNotPresent | ||
image: yarp/yarp-controller |
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.
Sadly we can't check in a reference to your image here for security reasons
@@ -36,8 +36,8 @@ spec: | |||
spec: | |||
containers: | |||
- name: yarp-proxy | |||
imagePullPolicy: Always | |||
image: <REGISTRY_NAME>/yarp:<TAG> | |||
imagePullPolicy: IfNotPresent |
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.
Out of curiosity, what's the significance of changing this policy? Wouldn't a cached image be used if one exists regardless?
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.
Always
will first query public feeds (e.g. Docker Hub, ACR, etc.) before checking locally.
When building and deploying locally, it's preferable to check locally first, before falling back to a public feed.
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.
Wouldn't this mean that if the image on a public feed is updated, you would not see changes locally?
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.
For repeatability, it is better to choose when to update which versions of images are being run inside your cluster.
This ensures that you "know" exactly what is running inside your cluster. Think secure supply chain, image scanning pipelines, etc.
fed41cb
to
00e1789
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.
Thanks!
Fixes #1280