BuildDefaults don't show up in Environment of BuildConfigs or Builds

[sdodson]

It's fairly opaque to end users that there are BuildDefaults or BuildOverrides being defined for them. It'd be nice if these were somehow displayed in the Environment section of BuildConfigs and Builds.

Version

oc v3.2.0.20 kubernetes v1.2.0-36-g4a3f9c5

Steps To Reproduce

1. Provision an environment with BuildDefaults admission controller defined.
2. Create a buildconfig, build it.

   Current Result

Injected values such as HTTPS_PROXY and HTTP_PROXY are not visible in the Environment tabs

Expected Result

Injected values visible in at least the actual Build's Environment tab if not the BuildConfig's Environment tab.

[bparees]

@csrwng what was our reasoning for just updating the build definition env variable in the pod, instead of updating the build object or buildconfig itself? concerns that users would undo the override? We could always do the override everywhere.

[csrwng]

@bparees @sdodson the reason was that we wanted to preserve the original intent of the user in the build object. For example, if your policy changes later (you setup a new proxy). For example you export your build object to another cluster or clone your build after you've changed settings. If the setting is a build default, we won't change what's in the build already and the setting will be wrong.

[sdodson]

Hmm, with that in mind I'm not sure what the right thing to do is. It definitely makes sense not to include it in the buildconfig but I think it should actually be part of the build so that if you export the build it's exactly as it was executed.

[csrwng]

@sdodson not sure... you may want to re-run an existing build and not a new one from build config (that's what oc start-build --from-build=BUILD does). Admittedly, it would be a less common use case. I'm thinking that maybe we should record in the Status part of the build what it actually ran with. That way you know what was actually used but don't change the original intent of the build. @bparees thoughts?

[bparees]

@csrwng sounds reasonable.

[csrwng]

@smarterclayton would you support changing the build status API struct to include env values that were actually used during the build?

[smarterclayton]

If this is for things that are used by the build, they should be set into the build spec. Not build status.

[bparees]

@smarterclayton we're trying to avoid putting them in the spec because of the following scenario:

1) admin sets up a builddefaulter that sets env FOO=BAR 2) user launched a build, build runs with FOO=BAR 3) admin changes defaulter to FOO=BAR2 4) user "rebuilds" the build from (2), but because FOO=BAR is already in the spec, they don't get the new default of FOO=BAR2. (the builddefaulter won't override existing env vars because it's a defaulter, not an overrider)

[csrwng]

I guess it makes a little bit more sense if s/FOO/HTTP_PROXY/

[smarterclayton]

The use case is pretty familiar, I'm just really hesitant to add a whole API feature for something like this. I would slightly prefer a build defaults spec field.

[smarterclayton]

This definitely feels more spec than status.

[bparees]

I would slightly prefer a build defaults spec field.

i kinda hate anything that's going to become a duplicate of all the fields that already exist in the build spec, which is what that (or the status solution) are going to become as we add more things that can be defaulted.

[smarterclayton]

What other things would we default / fill out?

On Fri, Jun 10, 2016 at 4:31 PM, Ben Parees notifications@github.com wrote:

I would slightly prefer a build defaults spec field.

i kinda hate anything that's going to become a duplicate of all the fields that already exist in the build spec, which is what that (or the status solution) are going to become as we add more things that can be defaulted.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/8663#issuecomment-225287887, or mute the thread https://github.com/notifications/unsubscribe/ABG_p9xLuqORTpeql38DmB6-hLFZHpBVks5qKcmXgaJpZM4ISBkZ .

[bparees]

force pull (currently an override), incremental, triggers (github, generic, configchange)... i'd have to look at the build spec to see what else.

[smarterclayton]

Those are all things I would expected to get defaulted on the object, and on rebuild be reused.

On Fri, Jun 10, 2016 at 4:53 PM, Ben Parees notifications@github.com wrote:

force pull (currently an override), incremental, triggers (github, generic, configchange)... i'd have to look at the build spec to see what else.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/8663#issuecomment-225292822, or mute the thread https://github.com/notifications/unsubscribe/ABG_p6_yE3QG8ybbPChQl-2ExX-4xzkjks5qKc7BgaJpZM4ISBkZ .

[bparees]

Those are all things I would expected to get defaulted on the object, and on rebuild be reused.

So you're making the argument that once the build is created, it should not pick up "changed" default values? (but it will pick up new default values). because if that's the case, we can just modify the build spec itself, we don't need a separate "default" spec.

[jupierce]

Just a thought that it could be useful to treat environment variables and proxy information differently.

Proxy information being persisted in a build configuration seems counter-intuitive to me -- like persisting proxy information as part of a browser bookmark -- it artificially inhibits migration between environments. I think the real concern here is that the information is hard to come across.

Couldn't we have a BuildEnvironment plugin which was solely responsible for the proxy settings? The values would never leak into the build configurations and we could render the values in the UIs. For example:

• Web console: under Project Settings as a new "Server Settings" area of the Build Configuration tab.
• CLI: oc status, oc describe bc, or as informational build stdout message.

Removing proxies makes the behavior of the remaining plugins predictable an unproblematic, from my limited perspective.

• BuildDefaults: Values wind up encoded into the build unless overridden. Effectively a convenience method to avoid repeating yourself.
• BuildOverrides: Values always affect the build.

[bparees]

Couldn't we have a BuildEnvironment plugin which was solely responsible for the proxy settings? The values would never leak into the build configurations and we could render the values in the UIs. For example:

what you're describing is basically what the builddefaulter and overrider already do today. we could have the UI and CLI tools munge together the override/default config settings along with the build/buildconfig object, to display to the user "what you're going to get" instead of "what you defined"

[jupierce]

@bparees Agreed. I was primarily arguing that any implementation of Defaults/Overrides makes more sense to me if the proxy information is extracted into a new plugin. Secondarily that the "simple" approach of stamping the build with the contemporaneous Defaults (i.e. without the need to munge) becomes fairly intuitive if we do.

[csrwng]

@bparees @jupierce did we agree on how to proceed with this issue (short/longer term)? In the short term, can we simply resolve it in the UI? If a build exists and the pod that it ran with exists, then we can extract the info from the pod and highlight the differences ? (actual vs spec). We could do this in the web console and the 'oc describe' output.

[bparees]

@csrwng UI gymnastics doesn't feel like the right answer. adding explicit "values from override/defaults" fields to the build seems like the right answer, much as i don't want to duplicate a bunch of fields.

[csrwng]

@bparees and I'm guessing we would like this change for 3.3 ? :)

[bparees]

@csrwng i don't think we want to try to do that, no.

[csrwng]

@bparees sending this one to you since what you're working on would hopefully fix this.

[bparees]

@csrwng i'm not actually going to be fixing this as part of what i'm doing, but the right fix for this in the future is, imho: we need to get more discipline around spec vs status, and builddefaults/overrides should only affect status, not spec. Then the user can see what they asked for (spec) and what they got (status). And they won't be able to change the status, so they won't be able to modify the values set by an override.

would need to be part of a v2 api of course.

wdyt?

[csrwng]

sgtm

[bparees]

trello'd https://trello.com/c/oLPEjyle