-
Notifications
You must be signed in to change notification settings - Fork 202
Keep BuildConfig configuration #1071
Keep BuildConfig configuration #1071
Conversation
@nicolaferraro could you please? :) |
While reviewing the commit, please check if my comment on the issue, if the fix really makes sens : #1070 (comment) |
I've maybe misunderstood the plugin's philosophy and if this is the case I'm sorry. But if not is it possible to help me finding a way to customize |
Any news on this one ? |
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.
Don't think this PR works as expected.
.withStrategy(buildStrategy) | ||
.withOutput(buildOutput) | ||
.withStrategy(spec.getStrategy()) | ||
.withOutput(spec.getOutput()) |
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.
I'm really not sure why this should fix something, as this is a no-op after all: You are re-setting the same strategy and output as it was there already. The whole idea of this method is to change the strategy (docker or binary s2i build) and the output stream if changed.
@zonArt could you please elaborate what are you trying to achieve ?
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.
@rhuss : I read the related issue filed by @zonArt , I think he wants to have his pushSecrets kept in BuildConfig
to push/pull from some private registry, maybe that's why he is setting same strategy and output itself.
Agreed that this doesn't align with plugin's flow at all. But is there some way to configure plugin to include these pushSecrets into BuildConfig?
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.
@rohanKanojia You got it totally right, being able to keep the configuration of the buildconfig and imagestream was the needed behavior thus this PR which was maybe not intended to be merged as is but more a way to request support on how to customize those objects or to find a way to keep provided ones (maybe with a flag)
Here another proposal, to add pushSecret and pullSecret to the builld config. master...ElcaIT:Feature/ThirdPartyRepositoryWithSecret However, this probably need some rework :
|
@zonArt @FlorianBois Let me try to somerise the issue you are trying to solve. It's all about being able to configure pull and push secrets for an OpenShift S2I build, right ? If so, we should indeed extend the configuration and implement this like @FlorianBois suggests. I suggest to close this PR and work on the diff referenced above. @FlorianBois could you please create a PR and we can then discuss over there about the naming, timeout configuration and registry handling ? |
If wanting to update
buildConfig
with existing oneit could be good to keep the spec defined in it otherwise it
is the same than the recreate option
May fix #1054 and #1070