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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add variable expansion in manifest.build.dockerfile #4217

Merged

Conversation

jLopezbarb
Copy link
Contributor

Proposed changes

This PR adds support for variable expansion on the expanded syntax of build.Dockerfile and build.Context

How to validate

  1. Create the following manifest:
build:
  app:
    context: .
    dockerfile: $DOCKERFILEPATH
  1. Run the command export DOCKERFILEPATH=Dockerfile
  2. Run okteto build

CLI Quality Reminders 馃敡

For both authors and reviewers:

  • Scrutinize for potential regressions
  • Ensure key automated tests are in place
  • Build the CLI and test using the validation steps
  • Assess Developer Experience impact (log messages, performances, etc)
  • If too broad, consider breaking into smaller PRs
  • Adhere to our code style and code review guidelines

Signed-off-by: Javier Lopez <javier@okteto.com>
@jLopezbarb jLopezbarb requested a review from a team as a code owner April 4, 2024 12:26
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Merging #4217 (9d6b475) into master (27e1d9c) will decrease coverage by 0.01%.
Report is 6 commits behind head on master.
The diff coverage is 33.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4217      +/-   ##
==========================================
- Coverage   45.64%   45.63%   -0.01%     
==========================================
  Files         298      298              
  Lines       27262    27266       +4     
==========================================
  Hits        12443    12443              
- Misses      13762    13764       +2     
- Partials     1057     1059       +2     

Copy link
Contributor

@maroshii maroshii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM based on how we currently do variable expansion.

As a side comment and for future reference, I don't think it scales to change these upon request. Why would we expand Dockerfile and Context and not Target and Image? I would expect a parser to generically expand every field generically

@maroshii
Copy link
Contributor

maroshii commented Apr 5, 2024

Will this need documentation?

@jLopezbarb jLopezbarb merged commit 3d623fa into master Apr 9, 2024
15 of 17 checks passed
@jLopezbarb jLopezbarb deleted the jlopezbarb/add-variable-extension-in-build-manifest branch April 9, 2024 10:59
@ifbyol
Copy link
Member

ifbyol commented Apr 9, 2024

@jLopezbarb Do we need to backport this to release-2.26?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants