-
Notifications
You must be signed in to change notification settings - Fork 39
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
Support ContainerEnvironmentVariables #181
Conversation
Note to self: docs link https://github.com/opencontainers/image-spec/blob/main/config.md |
1146f39
to
67286a2
Compare
@@ -220,6 +220,10 @@ public void Label(string name, string value) | |||
|
|||
public void AddEnvironmentVariable(string envVarName, string value) | |||
{ | |||
if (environmentVariables.ContainsKey(envVarName)) |
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.
We should overwrite here and make folks aware that this is the semantic. Otherwise, people can't do things like overwriting the ASPNETCORE_URLS
variable on their own, since the dotnet/runtime-deps
container includes that definition by default.
This one's new.
|
Turns out it was the default value we were setting ASPNETCORE_URLS to... I stole the value it is set to by default: |
3262d48
to
cbd92a0
Compare
Cleaned up the commit history, should be ready to merge. Un-drafting |
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.
Nice work! Thanks for knocking this out.
Fixes #179
Changes Made
Containerize, CreateNewImage, CreateNewImage (the tool task), and the targets have been modified to flow Environment Variables.
Also added a unit test that verifies it works by Console.Writing the value of the environment variable manually passed into the ParseContainerProeprties task.
Recommend a commit-by-commit review