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

[DNM] Prevent RUN_IMAGE from being overridden in build template #22

Closed
wants to merge 1 commit into from

Conversation

glyn
Copy link

@glyn glyn commented Jan 29, 2019

The build template parameter RUN_IMAGE no longer needs to be overrideable now that PR projectriff/riff#1082 is merged. This PR makes sure the default value cannot be overridden.

Downsides of this PR are:

  • the default value is now coded later in the YAML and a developer might overlook that
  • the image detection logic will need updating to relocate the hard-coded image

Ref projectriff/riff#1081

@glyn glyn requested review from ericbottard and scothis January 29, 2019 09:53
@glyn glyn self-assigned this Jan 29, 2019
@ericbottard
Copy link
Contributor

the image detection logic will need updating to relocate the hard-coded image
wouldn't it already detect that (because it looks like an image coordinate)?

@glyn
Copy link
Author

glyn commented Jan 29, 2019

the image detection logic will need updating to relocate the hard-coded image
wouldn't it already detect that (because it looks like an image coordinate)?

No, because the image detection scanner only looks for images in specific contexts and with specific clues. In general, image ids are hard to spot (e.g. "ubuntu") without other clues.

@ericbottard
Copy link
Contributor

So we need to wait for an update to that before merging this, I assume

@glyn
Copy link
Author

glyn commented Jan 29, 2019

So we need to wait for an update to that before merging this, I assume

Yes. However, I'm not going to implement the scanner change just yet in case we throw out this PR. Once this PR is approved, I'll beef up the scanner and then merge this PR. Until the scanner is beefed up, I'll keep DNM in the title of this PR.

@ericbottard
Copy link
Contributor

Eric approves :)

Copy link
Member

@scothis scothis left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this, I'd actually like to go the opposite direction and add the builder images as a param. riff generally should not set these params, but they are still useful as params.

  1. it's easier to update the image values as there is only one spot
  2. it's easier for tools to lookup the image value

scothis added a commit to scothis/builder that referenced this pull request 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 projectriff#22
@scothis
Copy link
Member

scothis commented Jan 29, 2019

@glyn @ericbottard I opened #23 which runs in the opposite direction of this PR by also parameterizing the run and util images.

@glyn
Copy link
Author

glyn commented Jan 30, 2019

I'm comfortable keeping the parameterisation.

@glyn glyn closed this Jan 30, 2019
@glyn glyn removed the in progress label Jan 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants