Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

Conversation

@aliok
Copy link
Member

@aliok aliok commented Mar 25, 2020

Issue to be fixed

Fixes #147

Proposed Changes

  • Also override images in env vars

Release Note

NONE

To override images in env vars, simply add the env var name as the key in the override map.

Ex:

Deployment

apiVersion: apps/v1
kind: Deployment
metadata:
  name: broker-controller
  namespace: knative-eventing
spec:
  template:
    spec:
      serviceAccountName: eventing-controller
      containers:
      - name: broker-controller
        terminationMessagePolicy: FallbackToLogsOnError
        image: gcr.io/knative-releases/knative.dev/eventing/cmd/channel_broker@sha256:853e0d194223da4356af7c9a17c79afa0ef96b93fd956d5febb2b1f2be78c717
        env:
        - name: BROKER_INGRESS_IMAGE
          value: gcr.io/knative-releases/knative.dev/eventing/cmd/broker/ingress@sha256:d080653ce0792a304ba388dcb1e360dc427d8556503e0a2603da26d261ac21e4

CR

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
...
spec:
  registry:
    override:
      broker-controller: docker.io/my-broker-controller
      BROKER_INGRESS_IMAGE : docker.io/my-broker-ingress

Result:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: broker-controller
  namespace: knative-eventing
spec:
  template:
    spec:
      serviceAccountName: eventing-controller
      containers:
      - name: broker-controller
        terminationMessagePolicy: FallbackToLogsOnError
        image: docker.io/my-broker-controller
        env:
        - name: BROKER_INGRESS_IMAGE
          value: docker.io/my-broker-ingress

Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@aliok: 0 warnings.

Details

In response to this:

Issue to be fixed

Fixes #147

Proposed Changes

  • Also override images in env vars

Release Note

NONE

To override images in env vars, simply add the env var name as the key in the override map.

Ex:

Deployment

apiVersion: apps/v1
kind: Deployment
metadata:
 name: broker-controller
 namespace: knative-eventing
spec:
 template:
   spec:
     serviceAccountName: eventing-controller
     containers:
     - name: broker-controller
       terminationMessagePolicy: FallbackToLogsOnError
       image: gcr.io/knative-releases/knative.dev/eventing/cmd/channel_broker@sha256:853e0d194223da4356af7c9a17c79afa0ef96b93fd956d5febb2b1f2be78c717
       env:
       -
         name: BROKER_INGRESS_IMAGE
         value: gcr.io/knative-releases/knative.dev/eventing/cmd/broker/ingress@sha256:d080653ce0792a304ba388dcb1e360dc427d8556503e0a2603da26d261ac21e4

CR

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
...
spec:
 registry:
   override:
     broker-controller: docker.io/my-broker-controller
     BROKER_INGRESS_IMAGE : docker.io/my-broker-ingress

Result:

apiVersion: apps/v1
kind: Deployment
metadata:
 name: broker-controller
 namespace: knative-eventing
spec:
 template:
   spec:
     serviceAccountName: eventing-controller
     containers:
     - name: broker-controller
       terminationMessagePolicy: FallbackToLogsOnError
       image: docker.io/my-broker-controller
       env:
       -
         name: BROKER_INGRESS_IMAGE
         value: docker.io/my-broker-ingress

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/knativeeventing/common/images.go 89.2% 91.1% 1.9

@aliok
Copy link
Member Author

aliok commented Mar 26, 2020

@aliok
Copy link
Member Author

aliok commented Mar 26, 2020

Note that #148 and this PR is related but they're separate.

This PR is about fixing the existing logic to override images in env vars.
Issue #148 is about improving it to support non-unique container names when overriding.

Copy link

@houshengbo houshengbo left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, houshengbo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markusthoemmes
Copy link
Contributor

/retest

Boskos failures.

@aliok
Copy link
Member Author

aliok commented Mar 27, 2020

/retest

@aliok
Copy link
Member Author

aliok commented Mar 27, 2020

To answer a question asked offline:;
With this PR we are able to do overriding with registry.override for env vars but it doesn't enable registry.default. That's actually not possible since registry.default requires knowing the container names.

For example, registry.override is like com.my.registry/${NAME} and the operator should replace the ${NAME} with the container name. It works for container names like eventing-controller as it ends up com.my.registry/eventing-controller. But for env var image overrides, we don't know the container name and replacing ${NAME} with the env var name won't work.

cc @houshengbo

@knative-prow-robot knative-prow-robot merged commit eaf1553 into knative:master Mar 27, 2020
@aliok aliok deleted the image-in-env-var-override branch March 30, 2020 08:16
aliok added a commit to aliok/eventing-operator that referenced this pull request Mar 30, 2020
aliok added a commit to openshift-knative/eventing-operator that referenced this pull request Mar 30, 2020
aliok added a commit to aliok/eventing-operator that referenced this pull request Mar 31, 2020
aliok added a commit to openshift-knative/eventing-operator that referenced this pull request Mar 31, 2020
houshengbo pushed a commit to houshengbo/eventing-operator that referenced this pull request Mar 31, 2020
houshengbo pushed a commit to houshengbo/eventing-operator that referenced this pull request Mar 31, 2020
houshengbo pushed a commit that referenced this pull request Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to override image for broker-ingress with operator

6 participants