Skip to content

[WIP] Add base image from the build UpstreamImage if set #561

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

Closed
maria wants to merge 1 commit into from
Closed

[WIP] Add base image from the build UpstreamImage if set #561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/build/api/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ type BuildParameters struct {

// Output describes the Docker image the Strategy should produce.
Output BuildOutput `json:"output,omitempty" yaml:"output,omitempty"`

UpstreamImage string `json:"upstreamImage,omitempty" yaml:"upstreamImage,omitempty"`
Copy link
Contributor

@smarterclayton smarterclayton Dec 13, 2014

Choose a reason for hiding this comment

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

This doesn't belong here, but on individual strategies. Not all strategies will have images.

Copy link
Contributor

@bparees bparees Dec 14, 2014

Choose a reason for hiding this comment

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

See comment here: https://github.com/openshift/origin/pull/557/files#r21797001

Copy link
Contributor

@smarterclayton smarterclayton Dec 17, 2014

Choose a reason for hiding this comment

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

I really don't want to refer to this as an upstream image in our api. I want it to align with the terminology for deployments (the reason why a build is occurring) and the terminology for each strategy that aligns with Docker. So that's BaseImage for Docker and BuilderImage for STI

Copy link
Contributor

@smarterclayton smarterclayton Dec 17, 2014

Choose a reason for hiding this comment

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

One more unrelated note for Ben - the image change triggers should probably be image name based - ie the trigger is on the docker pull spec "foo/bar:tag", so if you have two references to that we would replace both. That solves the complexity of reference by structure by making it the responsibility of generic code

Copy link
Contributor

@bparees bparees Dec 17, 2014

Choose a reason for hiding this comment

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

Yes, we know we're going to have to refactor this PR once the dust settles on my changes, so don't worry about the exact parameter/config definition here.

the image change triggers are going to work the same way the deployment ones work today, which means a reference to an imagerepository (imagestream).

Copy link
Contributor

@smarterclayton smarterclayton Dec 17, 2014

Choose a reason for hiding this comment

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

On Dec 17, 2014, at 12:02 PM, Ben Parees [email protected] wrote:

In pkg/build/api/types.go:

@@ -37,6 +37,8 @@ type BuildParameters struct {

// Output describes the Docker image the Strategy should produce.
Output BuildOutput `json:"output,omitempty" yaml:"output,omitempty"`
  • UpstreamImage string json:"upstreamImage,omitempty" yaml:"upstreamImage,omitempty"
    Yes, we know we're going to have to refactor this PR once the dust settles on my changes, so don't worry about the exact parameter/config definition here.

the image change triggers are going to work the same way the deployment ones work today, which means a reference to an imagerepository (imagestream).

Right, but they should identify what pull spec in the build strategy they work on as well. Image repo name doesn't have to equal pull spec in strategy. Indirection here allows the ability to work with sources under different control
—
Reply to this email directly or view it on GitHub.

Copy link
Contributor

@bparees bparees Dec 17, 2014

Choose a reason for hiding this comment

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

yes, the build trigger is going to be a conduit from "watch this imagestream:tag for changes" to "rewrite the buildconfig to use the new imageid reported by the imagestream"

}

// BuildStatus represents the status of a build at a point in time.
Expand Down
2 changes: 2 additions & 0 deletions pkg/build/api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ type BuildParameters struct {

// Output describes the Docker image the Strategy should produce.
Output BuildOutput `json:"output,omitempty" yaml:"output,omitempty"`

UpstreamImage string `json:"upstreamImage,omitempty" yaml:"upstreamImage,omitempty"`
}

// BuildStatus represents the status of a build at a point in time.
Expand Down
65 changes: 39 additions & 26 deletions pkg/build/builder/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/url"
"os"
"path/filepath"
"regexp"
"strings"
"time"

Expand Down Expand Up @@ -54,10 +55,10 @@ func (d *DockerBuilder) Build() error {
if err = d.fetchSource(buildDir); err != nil {
return err
}
if err = d.dockerBuild(buildDir); err != nil {
if err = d.addBuildParameters(buildDir); err != nil {
return err
}
if err = d.addImageVars(); err != nil {
if err = d.dockerBuild(buildDir); err != nil {
return err
}
if d.build.Parameters.Output.Registry != "" || d.authPresent {
Expand Down Expand Up @@ -125,40 +126,52 @@ func (d *DockerBuilder) fetchSource(dir string) error {
return d.git.Checkout(dir, d.build.Parameters.Source.Git.Ref)
}

// dockerBuild performs a docker build on the source that has been retrieved
func (d *DockerBuilder) dockerBuild(dir string) error {
var noCache bool
if d.build.Parameters.Strategy.DockerStrategy != nil {
if d.build.Parameters.Strategy.DockerStrategy.ContextDir != "" {
dir = filepath.Join(dir, d.build.Parameters.Strategy.DockerStrategy.ContextDir)
}
noCache = d.build.Parameters.Strategy.DockerStrategy.NoCache
}
return buildImage(d.dockerClient, dir, noCache, imageTag(d.build), d.tar)
}
// addBuildParameters checks if an UpstreamImage is set to replace the default base image.
// If that's the case then change the Dockerfile to make the build with the given image.
// Also append the environment variables in the Dockerfile.
func (d *DockerBuilder) addBuildParameters(dir string) error {
dockerfilePath := filepath.Join(dir, d.build.Parameters.Strategy.DockerStrategy.ContextDir, "Dockerfile")

// addImageVars creates a new Dockerfile which adds certain environment
// variables to the previously tagged image
func (d *DockerBuilder) addImageVars() error {
var noCache bool
envVars := getBuildEnvVars(d.build)
tempDir, err := ioutil.TempDir("", "overlay")
fileStat, err := os.Lstat(dockerfilePath)
filePerm := fileStat.Mode()

fileData, err := ioutil.ReadFile(dockerfilePath)
if err != nil {
return err
}
overlay, err := os.Create(filepath.Join(tempDir, "Dockerfile"))
if err != nil {
return err

var newFileData string

if d.build.Parameters.UpstreamImage != "" {
re := regexp.MustCompile(`^FROM \w+.+/+\w+`)
newFileData = re.ReplaceAllString(string(fileData), fmt.Sprintf("FROM %s\n", d.build.Parameters.UpstreamImage))
}
overlay.WriteString(fmt.Sprintf("FROM %s\n", imageTag(d.build)))

envVars := getBuildEnvVars(d.build)
for k, v := range envVars {
overlay.WriteString(fmt.Sprintf("ENV %s %s\n", k, v))
newFileData = newFileData + fmt.Sprintf("ENV %s %s\n", k, v)
}
if err = overlay.Close(); err != nil {

err = ioutil.WriteFile(dockerfilePath, []byte(newFileData), filePerm)
if err != nil {
return err
}

var noCache bool
if d.build.Parameters.Strategy.DockerStrategy != nil {
noCache = d.build.Parameters.Strategy.DockerStrategy.NoCache
}
return buildImage(d.dockerClient, tempDir, noCache, imageTag(d.build), d.tar)
return buildImage(d.dockerClient, dir, noCache, imageTag(d.build), d.tar)
}

// dockerBuild performs a docker build on the source that has been retrieved
func (d *DockerBuilder) dockerBuild(dir string) error {
var noCache bool
if d.build.Parameters.Strategy.DockerStrategy != nil {
if d.build.Parameters.Strategy.DockerStrategy.ContextDir != "" {
dir = filepath.Join(dir, d.build.Parameters.Strategy.DockerStrategy.ContextDir)
}
noCache = d.build.Parameters.Strategy.DockerStrategy.NoCache
}
return buildImage(d.dockerClient, dir, noCache, imageTag(d.build), d.tar)
}
7 changes: 6 additions & 1 deletion pkg/build/builder/sti.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,18 @@ func NewSTIBuilder(client DockerClient, dockerSocket string, authCfg docker.Auth
// Build executes the STI build
func (s *STIBuilder) Build() error {
request := &sti.STIRequest{
BaseImage: s.build.Parameters.Strategy.STIStrategy.Image,
DockerSocket: s.dockerSocket,
Source: s.build.Parameters.Source.Git.URI,
Tag: imageTag(s.build),
Environment: getBuildEnvVars(s.build),
Clean: s.build.Parameters.Strategy.STIStrategy.Clean,
}

if s.build.Parameters.UpstreamImage != "" {
request.BaseImage = s.build.Parameters.UpstreamImage
} else {
request.BaseImage = s.build.Parameters.Strategy.STIStrategy.Image
}
if s.build.Parameters.Revision != nil && s.build.Parameters.Revision.Git != nil &&
s.build.Parameters.Revision.Git.Commit != "" {
request.Ref = s.build.Parameters.Revision.Git.Commit
Expand Down