Skip to content
This repository has been archived by the owner on Jun 19, 2024. It is now read-only.

Keep BuildConfig configuration #1071

Closed
wants to merge 1 commit into from
Closed

Keep BuildConfig configuration #1071

wants to merge 1 commit into from

Conversation

zonArt
Copy link

@zonArt zonArt commented Oct 17, 2017 •
edited
Loading

If wanting to update buildConfig with existing one
it could be good to keep the spec defined in it otherwise it
is the same than the recreate option
May fix #1054 and #1070

Potential fix for #1054 and #1070
fbe5cbe 
If wanting to update buildConfig with existing one
it could be good to keep the spec defined in it otherwise it
is the same than the recreate option
Fix #1054 and #1070
@zonArt zonArt mentioned this pull request Oct 18, 2017
Closed
@hrishin
Copy link
Member

hrishin commented Oct 23, 2017 •
edited
Loading

@nicolaferraro could you please? :)

@FlorianBois
Copy link

While reviewing the commit, please check if my comment on the issue, if the fix really makes sens : #1070 (comment)

@zonArt
Copy link
Author

zonArt commented Oct 30, 2017

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 BuildConfigs and ImageStreams within f-m-p ?
Thanks

@zonArt
Copy link
Author

zonArt commented Nov 22, 2017

Any news on this one ?

@rohanKanojia rohanKanojia added the pr/waiting-on-feedback Waiting on feedback label Jul 24, 2018
@rohanKanojia rohanKanojia requested a review from rhuss July 24, 2018 05:49
@rohanKanojia rohanKanojia added the target/3.5 PR targeted to 3.5 label Jul 24, 2018
rhuss
rhuss suggested changes Jul 24, 2018
View reviewed changes
Copy link
Contributor

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

core/src/main/java/io/fabric8/maven/core/service/openshift/OpenshiftBuildService.java
.withStrategy(buildStrategy)
.withOutput(buildOutput)
.withStrategy(spec.getStrategy())
.withOutput(spec.getOutput())
Copy link
Contributor

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 ?

Copy link
Member

@rohanKanojia rohanKanojia Jul 25, 2018 •
edited
Loading

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?

Copy link
Author

@zonArt zonArt Jul 25, 2018 •
edited
Loading

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)

@rohanKanojia rohanKanojia added pr/change-requested Reviewer requested a change and removed pr/change-requested Reviewer requested a change pr/waiting-on-feedback Waiting on feedback labels Jul 24, 2018
@FlorianBois
Copy link

FlorianBois commented Jul 25, 2018 •
edited
Loading

Here another proposal, to add pushSecret and pullSecret to the builld config.

master...ElcaIT:Feature/ThirdPartyRepositoryWithSecret

However, this probably need some rework :

  • The build time out of the client has been increased, because the default is to low, a proper PR with configurable timeout should also be welcome
  • this line withNewSpec().withDockerImageRepository(imageConfiguration.getRegistry()+"/"+imageName.getNameWithoutTag())
    on OpenshiftBuildService is highly to be wrongly coded, I took the registry from the imageConfiguration, but it is possible that it should come from an additional configuration or reuse another one (currently, there is many ways to tell the plugin there is a repository), taking the imageName without the image tag looks also more like a workaround than a real solution.

@rohanKanojia rohanKanojia added the pr/wip Work in Progress, do not merge label Jul 25, 2018
@rhuss rhuss changed the title Potential fix for #1054 and #1070 Keep BuildConfig configuration Jul 30, 2018
@rhuss
Copy link
Contributor

rhuss commented Jul 30, 2018

@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 ?

@rhuss rhuss closed this Jul 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rohanKanojia rohanKanojia rohanKanojia left review comments

@rhuss rhuss rhuss requested changes

@hrishin hrishin Awaiting requested review from hrishin

@nicolaferraro nicolaferraro Awaiting requested review from nicolaferraro

Assignees

@rohanKanojia rohanKanojia

Labels
pr/wip Work in Progress, do not merge target/3.5 PR targeted to 3.5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support DockerImage as output for Openshift builds
5 participants
@zonArt @hrishin @FlorianBois @rhuss @rohanKanojia