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.

Sign up for GitHub

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

Jump to bottom

New-app/new-build support for pipeline buildconfigs #11897

Merged
merged 1 commit into from
Nov 17, 2016
Merged

New-app/new-build support for pipeline buildconfigs #11897

merged 1 commit into from
Nov 17, 2016

Conversation

jim-minter
Copy link
Contributor

@jim-minter jim-minter force-pushed the trello123-pipeline-buildconfigs branch from 355e04f to 8be5c84 Compare November 14, 2016 13:17
@jim-minter
Copy link
Contributor Author

@bparees Last test failed due to kubernetes/kubernetes#32455 , but otherwise ptal

@bparees bparees added this to the 1.5.0 milestone Nov 14, 2016
bparees
bparees requested changes Nov 14, 2016
View reviewed changes
Copy link
Contributor

@bparees bparees left a comment

Choose a reason for hiding this comment

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

one question and also i think we need to consider throwing an error if someone tries to use the --binary flag with a pipeline strategy since that won't work.

pkg/generate/app/cmd/resolve.go

case info.Jenkinsfile && (g.Strategy == generate.StrategyUnspecified || g.Strategy == generate.StrategyPipeline):
refs := b.AddComponents([]string{"scratch"}, func(input *app.ComponentInput) app.ComponentReference {
input.Resolver = app.FirstMatchResolver{Searcher: app.DockerClientSearcher{}}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a resolver here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There may well be a better way to do this. I found the concept of a "scratch" component which doesn't seem to be much used, but was useful here because the code expects every repo to be attached to a component of some sort. In the other strategy cases the component to use is clear; here I'm using "scratch" as a fake component.

I set the resolver to be fairly minimal but able to respond to the request for "scratch". I wasn't clear that there was a neater way of doing this without a resolver but I'm all ears.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, a "cleaner" (but more work) solution might be to introduce a new resolver implementation that returns an appropriate pipeline component match.

"scratch" is really intended to refer to cases where the base image is the docker "scratch" image (eg a dockerfile with "FROM scratch" so i'm a little worried that overloading the use here could lead to fragility if the actual scratch codepath gets changed later.

@jim-minter jim-minter force-pushed the trello123-pipeline-buildconfigs branch from 8be5c84 to 998c2bb Compare November 15, 2016 11:28
@jim-minter
Copy link
Contributor Author

@bparees thanks - all changes made as specified.

bparees
bparees reviewed Nov 15, 2016
View reviewed changes
pkg/generate/app/cmd/resolve.go
return nil, errors.New("when --strategy is specified you must provide at least one source code location")
}

if g.BinaryBuild && (len(repositories) > 0 || components.HasSource()) {
return nil, errors.New("specifying binary builds and source repositories at the same time is not allowed")
}

if g.BinaryBuild && g.Strategy == generate.StrategyPipeline {
return nil, errors.New("specifying binary builds and the pipeline strategy at the same time is not allowed")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can this check not be done in the cmd validation logic? (newapp.CompleteAppConfig() ). That seems like the more appropriate location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jim-minter jim-minter force-pushed the trello123-pipeline-buildconfigs branch from 998c2bb to 91cdfc4 Compare November 15, 2016 18:09
@bparees
Copy link
Contributor

bparees commented Nov 15, 2016

lgtm for post 3.4

@bparees
Copy link
Contributor

bparees commented Nov 15, 2016

failure looks legitimate:

--- FAIL: TestNewAppRunBuilds (4.90s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0xa0 pc=0x109bb1b]

@jim-minter jim-minter force-pushed the trello123-pipeline-buildconfigs branch from 91cdfc4 to 1226bd1 Compare November 15, 2016 20:04
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 1226bd1

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11462/) (Base Commit: f9f9e9c)

@bparees
Copy link
Contributor

bparees commented Nov 16, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 1226bd1

@wanghaoran1988
Copy link
Member

@jim-minter Do you think whether we need add a test scenario that the context dir of the repo contains both Dockerfile and Jenkinsfile, and I am not sure which kind of the build is our expected one.

@bparees
Copy link
Contributor

bparees commented Nov 17, 2016

i think we decided the dockerfile should take precedence (retains backwards compatibility, sort of) and you use --strategy if you want a pipeline instead.

@bparees
Copy link
Contributor

bparees commented Nov 17, 2016

(but yes the scenario should be tested)

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 17, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11522/) (Base Commit: 2450c32) (Image: devenv-rhel7_5374)

@openshift-bot openshift-bot merged commit 027b95f into openshift:master Nov 17, 2016
@jim-minter
Copy link
Contributor Author

Hmm, in the case of unspecified strategy, I think what I've implemented is: if there's a Dockerfile, use it. If not and there's a Jenkinsfile, use it. Else if there's recognised bare source, use it (i.e. "Dockerfile > Jenkinsfile > bare source"). See AddMissingComponentsToRefBuilder().

Previously it was: if there's a Dockerfile, use it. Else if there's recognised bare source, use it (i.e. "Dockerfile > bare source").

I don't think that interposing Jenkinsfile in "Dockerfile > Jenkinsfile > bare source" can really be said to be maintaining backwards compatibility (it does for the Dockerfile case, but not for the bare source case).

I think that arguably we should go (have gone?) for "Jenkinsfile > Dockerfile > bare source" ("pipelines are now king") or perhaps "Dockerfile > bare source > Jenkinsfile" ("backwards compatibility is king").

I think that either is arguably preferential to what I implemented, but it's hard to choose between them and I'm not sure how much it matters to change it anyway. @bparees could you make a judgement call?

@bparees
Copy link
Contributor

bparees commented Nov 21, 2016

I agree it's not clear what's best and no way to maintain true compatibility.

jenkinsfile -> dockerfile -> source seems reasonable to me. As you say, making jenkinsfile king makes sense.

@jim-minter jim-minter mentioned this pull request Nov 21, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparees bparees bparees requested changes

Assignees

@bparees bparees

Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jim-minter @bparees @openshift-bot @wanghaoran1988