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

Container image build args can be overridden following their order of precedence #3000

Open
2 tasks
rohanKanojia opened this issue Apr 30, 2024 · 12 comments · May be fixed by #3637
Open
2 tasks

Container image build args can be overridden following their order of precedence #3000

rohanKanojia opened this issue Apr 30, 2024 · 12 comments · May be fixed by #3637
Assignees

Comments

@rohanKanojia
Copy link
Member

rohanKanojia commented Apr 30, 2024

Component

JKube Kit

Description

Originally discussed in #2994 (comment)

Currently, JKube tries to discover Docker Build Args from the following sources:

  • Build Args specified directly in ImageConfiguration
  • Build Args specified via System Properties
  • Build Args specified via Project Properties
  • Build Args specified via Plugin configuration
  • Docker Proxy Build Args detected from ~/.docker/config.json

At the moment BuildArgResolverUtil (introduced in #2994) throws an exception if the same keys are provided from different sources (mentioned above).

User's might want to override some of these build args on a single execution (for example while performing a single build from the command line).
To be able to allow for this, the behavior needs to be changed so that instead of throwing an exception in case of key collision, a warning is logged.

The proper order of precedence should be documented (and tested) too.

Acceptance Criteria

  • A warning is logged in case of same keys provided via different methods.
  • Precedence is documented in project documentation.
@rohanKanojia rohanKanojia changed the title BuildArgResolverUtil : Remove usage of com.google.common.collect.ImmutableMap BuildArgResolverUtil : Allow overriding build args from various sources Apr 30, 2024
@manusa manusa closed this as not planned Won't fix, can't repro, duplicate, stale Apr 30, 2024
@rohanKanojia
Copy link
Member Author

@manusa : We still need to change logic to allow overriding of build args. Current behavior in #2994 is to throw exception on if build args with same keys are provided.

@manusa manusa reopened this Apr 30, 2024
@manusa manusa changed the title BuildArgResolverUtil : Allow overriding build args from various sources Container image build args can be overridden following their precedence Apr 30, 2024
@manusa manusa changed the title Container image build args can be overridden following their precedence Container image build args can be overridden following their order of precedence Apr 30, 2024
@clarenced
Copy link
Contributor

Hi @rohanKanojia , do you still need help on this issue ?

@rohanKanojia
Copy link
Member Author

@clarenced : Sorry for the late reply, yes it would be nice to get this issue addressed. However, I'm not sure if it's clear enough for you to pick up. I'll check and get back to you.

@clarenced
Copy link
Contributor

@rohanKanojia I look at it this weekend and come later if have further questions :)

@rohanKanojia
Copy link
Member Author

@clarenced : Have you already started working on it?

@clarenced
Copy link
Contributor

@rohanKanojia Not yet, I just read the discussions to have an idea.

@l3002
Copy link
Contributor

l3002 commented Dec 29, 2024

Hi @rohanKanojia, It's been a while, do you mind if I pick this up?

@rohanKanojia
Copy link
Member Author

@l3002 : 👋 hey, nice to have you back! Sure, you can try this.

@l3002
Copy link
Contributor

l3002 commented Dec 30, 2024

thanks @rohanKanojia. I'll get started with it.

@l3002
Copy link
Contributor

l3002 commented Jan 1, 2025

Hi @rohanKanojia, Have we decided the precedence of the sources over one another? Because I believe that the following order of precedence (from higher to lower) would be the best possible one for implementing this:

  • Build Args specified directly in ImageConfiguration
  • Build Args specified via Project Properties
  • Build Args specified via System Properties
  • Build Args specified via Plugin configuration
  • Docker Proxy Build Args detected from ~/.docker/config.json

Let me know what you think about it.

@manusa
Copy link
Member

manusa commented Jan 7, 2025

What's the reason for changing the order of precedence stated by Rohan in the description of the issue?

I find it OK that project properties override system properties (although I'm not sure of the side effects especially with Gradle).

@l3002
Copy link
Contributor

l3002 commented Jan 7, 2025

Hi @manusa, I think precedence mentioned by Rohan is alright but I also think it would be better if project properties have a precedence over system properties as the user might want to declare more general args as env variables and project specific properties in project properties. I'm not sure about the side effects with build tools, though. I'll check for any anomalies or side effects and update.

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