Skip to content
This repository has been archived by the owner on Mar 13, 2021. It is now read-only.

Parameterize builder and util images in build template #23

Merged
merged 3 commits into from
Jan 31, 2019

Conversation

scothis
Copy link
Member

@scothis scothis commented Jan 29, 2019

The builder template is used twice within the build template, as is the
util image. We can centralize the definition of the builder image by
using a parameter. In general, builds should not override the run or
builder image params. It is still useful to parameterize them in order
to make it easier for tooling to discover or update the value cluster
wide.

Refs #22

The builder template is used twice within the build template, as is the
util image. We can centralize the definition of the builder image by
using a parameter. In general, builds should not override the run or
builder image params. It is still useful to parameterize them in order
to make it easier for tooling to discover or update the value cluster
wide.

Refs projectriff#22
Copy link

@glyn glyn left a comment

Choose a reason for hiding this comment

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

LGTM

default: projectriff/builder:latest
- name: UTIL_IMAGE
description: The util image provides pack utilities to analyze and export built images.
default: packs/util
Copy link
Member Author

Choose a reason for hiding this comment

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

should we use :latest or pin to a release, like :v3alpha1?

Copy link
Member

Choose a reason for hiding this comment

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

I would think a tag

Copy link
Member Author

Choose a reason for hiding this comment

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

turns out packs/util is deprecated

- replace util image with builder (packs/util is deprecated)
- move cache dir to /layers
- manually do what knative-helper did in the helper step
- merge riff.toml creation into the prepare step
- use flags instead of env vars
@scothis
Copy link
Member Author

scothis commented Jan 30, 2019

Adopted changes shown in https://github.com/knative/build-templates/pull/67/files#diff-21fea67d0c528b683d3f9785c45e38c7

  • replace util image with builder (packs/util is deprecated)
  • move cache dir to /layers
  • manually do what knative-helper did in the helper step
  • merge riff.toml creation into the prepare step
  • use flags instead of env vars

@scothis
Copy link
Member Author

scothis commented Jan 30, 2019

If desirable, we could also flatten each pack lifecycle step into a single container.

@trisberg
Copy link
Member

Trying this with the latest changes I get

2019/01/30 19:29:56 Group: Cloud Foundry OpenJDK Buildpack: pass | Node.js Buildpack: pass | Cloud Foundry Build System Buildpack: fail | NPM Buildpack: fail | riff Buildpack: pass
flag provided but not defined: -layers
Usage of /lifecycle/analyzer:
  -daemon
    	export to docker daemon
  -group string
    	path to group.toml (default "./group.toml")
  -helpers
    	use credential helpers
  -launch string
    	path to launch directory (default "/workspace")
  -metadata string
    	path to json containing image metadata for previous image

do we need to change projectriff/builder?

args:
- "-c"
- >
chown -R "${USER_ID}:${GROUP_ID}" "/builder/home" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment that explains why those 2 first lines are necessary? Never got to tracking that down

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a comment. The packs tasks run as the pack user, and still need to be able to read these files. This behavior used to be packaged up in the /lifecycle/knative-helper which has since been removed.

@scothis scothis merged commit 279a32f into projectriff:master Jan 31, 2019
@scothis scothis deleted the builder-param branch January 31, 2019 16:13
@nebhale nebhale added semver:patch A change requiring a patch version bump type:task A general task labels Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver:patch A change requiring a patch version bump type:task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants