Skip to content
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

Merged
merged 7 commits into from
Aug 28, 2024
Merged

Conversation

condar-metacell
Copy link
Contributor

@condar-metacell condar-metacell commented Aug 12, 2024

Closes CH-142

Implemented solution

A dockerfile section has been added under the haress section of an application which then has a buildArgs 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 run harness-deployment -i samples and skaffold dev to see the value set in the values echoed out during build.

Sanity checks:

  • The pull request is explicitly linked to the relevant issue(s)
  • The issue is well described: clearly states the problem and the general proposed solution(s)
  • In this PR it is explicitly stated how to test the current change
  • The labels in the issue set the scope and the type of issue (bug, feature, etc.)
  • The relevant components are indicated in the issue (if any)
  • All the automated test checks are passing
  • All the linked issues are included in one Sprint
  • All the linked issues are in the Review state
  • All the linked issues are assigned

Breaking changes (select one):

  • The present changes do not change the preexisting api in any way
  • This PR and the issue are tagged as a breaking-change and the migration procedure is well described above

Possible deployment updates issues (select one):

  • There is no reason why deployments based on CloudHarness may break after the current update
  • This PR and the issue are tagged as alert:deployment

Test coverage (select one):

  • Tests for the relevant cases are included in this pr
  • The changes included in this pr are out of the current test coverage scope

Documentation (select one):

  • The documentation has been updated to match the current changes
  • The changes included in this PR are out of the current documentation scope

Nice to have (if relevant):

  • Screenshots of the changes
  • Explanatory video/animated gif

@condar-metacell condar-metacell added python Pull requests that update Python code scope:deployment new feature labels Aug 12, 2024
Copy link
Contributor

@zsinnema zsinnema left a 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.

@filippomc
Copy link
Collaborator

filippomc commented Aug 26, 2024

@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

@filippomc filippomc self-requested a review August 26, 2024 13:08
Copy link
Collaborator

@filippomc filippomc left a 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]:
Copy link
Collaborator

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

Copy link
Contributor Author

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 :)

@zsinnema
Copy link
Contributor

@filippomc @condar-metacell did you see my comment here: https://metacell.atlassian.net/browse/CH-142?focusedCommentId=15219?

Copy link
Collaborator

@filippomc filippomc left a 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.

@filippomc filippomc merged commit aafb9a1 into develop Aug 28, 2024
6 of 7 checks passed
@filippomc filippomc deleted the feature/CH-142 branch August 28, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature python Pull requests that update Python code scope:deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants