8000
Skip to content

Allow S2I behavior to be overriden by caller#310

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
smarterclayton:add_binary_build
Oct 20, 2015
Merged

Allow S2I behavior to be overriden by caller#310
openshift-bot merged 1 commit intoopenshift:masterfrom
smarterclayton:add_binary_build

Conversation

@smarterclayton
Copy link
Copy Markdown
Contributor

Origin needs to be able to download and assemble the source
itself and then pass it to s2i. This commit adds changes to
allow an Overrides object to be passed to the s2i initializer
code to provide a custom Downloader.

This also makes changes to allow better reuse of upstream s2i
code and removes a write to disk for the intermediate tar (which
can be streamed instead).

@mfojtik @bparees review, this is part of binary build

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is dupe of line 61

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean - if no override is provided, we now create a default downloader and set it. b.source is only set once.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, you meant 61 in the old file. Fixed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i liked the old line 61 better than your new one. :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed it for you - including saving you from a govet error for anonymous
field declarations

On Mon, Oct 19, 2015 at 5:48 PM, Ben Parees notifications@github.com
wrote:

In pkg/build/strategies/onbuild/onbuild.go
#310 (comment)
:

s.SetScripts([]string{}, []string{api.Assemble, api.Run})
  • downloader, sourceUrl, err := scm.DownloaderForSource(config.Source)
  • if err != nil {
  •   return nil, err
    
  • downloader := overrides.Downloader
  • if downloader == nil {
  •   d, sourceURL, err := scm.DownloaderForSource(config.Source)
    
  •   if err != nil {
    
  •       return nil, err
    
  •   }
    
  •   downloader = d
    
  •   config.Source = sourceURL
    
  • }
  • b.source = onBuildSourceHandler{

i liked the old line 61 better than your new one. :)


Reply to this email directly or view it on GitHub
https://github.com/openshift/source-to-image/pull/310/files#r42430517.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks, now yours is better :)

@soltysh
Copy link
Copy Markdown
Contributor
soltysh commented Oct 14, 2015

One small nit, but I'd like to see tests for that please.

@bparees
Copy link
Copy Markdown
Contributor
bparees commented Oct 14, 2015

Assuming this hasn't changed much since I reviewed it in your origin PR I'm
ok with it.

Otherwise it's going to need to wait until Monday when I have laptop access
again (I guess android just isn't as good at doing code reviews as iOS...)

But +1 to TCs.

Ben Parees | OpenShift
On Oct 14, 2015 1:11 PM, "Clayton Coleman" notifications@github.com wrote:

Origin needs to be able to download and assemble the source
itself and then pass it to s2i. This commit adds changes to
allow an Overrides object to be passed to the s2i initializer
code to provide a custom Downloader.

This also makes changes to allow better reuse of upstream s2i
code and removes a write to disk for the intermediate tar (which
can be streamed instead).

@mfojtik https://github.com/mfojtik @bparees

https://github.com/bparees review, this is part of binary build

You can view, comment on, or merge this pull request online at:

#310
Commit Summary

  • Allow S2I behavior to be overriden by caller

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#310.

@smarterclayton
Copy link
Copy Markdown
Contributor Author

Why does STI execute take an api.Config that might be different from the api.Config passed to it on creation?

@smarterclayton
Copy link
Copy Markdown
Contributor Author

Tests added.

@smarterclayton smarterclayton force-pushed the add_binary_build branch 3 times, most recently from 25df5f4 to fe5fbf3 Compare October 18, 2015 14:48
@smarterclayton
Copy link
Copy Markdown
Contributor Author

Ok, race averted.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is duping this line isn't it?

@bparees
Copy link
Copy Markdown
Contributor
bparees commented Oct 19, 2015

squish and lgtm.

@smarterclayton
Copy link
Copy Markdown
Contributor Author

[merge]

On Mon, Oct 19, 2015 at 5:55 PM, Ben Parees notifications@github.com
wrote:

squish and lgtm.


Reply to this email directly or view it on GitHub
#310 (comment)
.

Origin needs to be able to download and assemble the source
itself and then pass it to s2i. This commit adds changes to
allow an Overrides object to be passed to the s2i initializer
code to provide a custom Downloader.

This also makes changes to allow better reuse of upstream s2i
code and removes a write to disk for the intermediate tar (which
can be streamed instead).
@smarterclayton
Copy link
Copy Markdown
Contributor Author

[merge]

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for source to image merge up to d31d44f

@openshift-bot
Copy link
Copy Markdown
Contributor

Source To Image Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_sti/196/)

openshift-bot pushed a commit that referenced this pull request Oct 20, 2015
@openshift-bot openshift-bot merged commit 08e6631 into openshift:master Oct 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

0