-
Notifications
You must be signed in to change notification settings - Fork 5
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
CH-142 Add dockerfile build args support to apps #763
Conversation
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.
@condar-metacell I left a comment for @filippomc in the jira ticket (https://metacell.atlassian.net/browse/CH-142?focusedCommentId=15079)
I'm not sure we should implement this feature.
Even if this is only for Skaffold I'm not sure why we need this. Hope that Filippo can explain the need.
@zsinnema this is useful for local development and debugging as mentioned in the Jira card, but I see your concern about this being abused as an attempt to set env variables on CI/CD and production systems. Let's be sure that the documentation states clearly that this feature shouldn't be used to set environment variables @condar-metacell |
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.
Implementation looks good to me (only one minor remark on arg typing).
The main issue for me is the lack of documentation that gives clarity about this feature and possible misuses (see other comment). Another point to remark in the docs will be the fact that the feature is only supported in Skaffold and not in Codefresh CI/CD. Honestly I don't like much the incoherence although I agree with @zsinnema that the use in CI/CD is not a use of this feature we want to promote.
|
||
|
||
def get_additional_build_args(helm_values: dict, app_key: str) -> dict[str, str]: |
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.
the typing is wrong as helm_values is not a dict. It's HarnessMainConfig
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.
Yep, thank you I'll adjust that to be correct :)
@filippomc @condar-metacell did you see my comment here: https://metacell.atlassian.net/browse/CH-142?focusedCommentId=15219? |
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.
LGTM thanks @condar-metacell for the additional documentation.
Closes CH-142
Implemented solution
A
dockerfile
section has been added under theharess
section of an application which then has abuildArgs
map which is provided to the dockerfile by Skaffold when building.How to test this PR
Either run
pytest
to run the added unit test, or runharness-deployment -i samples
andskaffold dev
to see the value set in the values echoed out during build.Sanity checks:
Breaking changes (select one):
breaking-change
and the migration procedure is well described abovePossible deployment updates issues (select one):
alert:deployment
Test coverage (select one):
Documentation (select one):
Nice to have (if relevant):