Avoid name conflict by using deployment+delimiter+container as the key #159

houshengbo · 2020-03-30T17:36:59Z

Issue to be fixed

Fixes #148

Proposed Changes

Release Note

knative-prow-robot

@houshengbo: 0 warnings.

In response to this:

Issue to be fixed

Fixes #

Proposed Changes

Release Note

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-prow-robot · 2020-03-30T17:37:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [houshengbo]

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

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor · 2020-03-30T18:14:26Z

pkg/reconciler/knativeeventing/common/images.go

@@ -33,6 +33,7 @@ import (
 var (
 	// The string to be replaced by the container name
 	containerNameVariable = "${NAME}"
+	delimiter = "-"


Format Go code:

Suggested change

delimiter = "-"

delimiter = "-"

Cynocracy · 2020-03-30T19:01:07Z

pkg/reconciler/knativeeventing/common/images.go

@@ -33,6 +33,7 @@ import (
 var (
 	// The string to be replaced by the container name
 	containerNameVariable = "${NAME}"
+	delimiter             = "-"


I thought the decision from chat was to use / as a delimiter, am I misremembering?

In particular, because - is allowed in deployment's container names, the override for

deployment-container

is ambiguous, it could mean container in deployment, or it could mean a container called deployment-container.

I got the issue. It looks like I need to switch to /.

Cynocracy · 2020-03-30T19:06:18Z

pkg/reconciler/knativeeventing/common/images.go

-func getNewImage(registry *eventingv1alpha1.Registry, containerName string) string {
-	overrideImage := registry.Override[containerName]
+func getNewImage(registry *eventingv1alpha1.Registry, containerName, deploymentName string) string {
+	overrideImage := registry.Override[deploymentName+delimiter+containerName]


Re-using the same override map for deployment and deployment-container style overrides could cause issues if the deployment name contains -, which seems to be possible.

Thoughts on creating two maps, one for container-only overrides and one for deployment+container?

I have changed the delimiter to /.

Cynocracy · 2020-03-30T19:11:43Z

pkg/reconciler/knativeeventing/common/images_test.go

@@ -172,6 +172,96 @@ var updateDeploymentImageTests = []updateDeploymentImageTest{
 			Env:   []corev1.EnvVar{{Name: "SOME_IMAGE", Value: "docker.io/my/overridden-image"}},
 		}},
 	},
+	{


It would be nice to test a few more corner cases:

container X in multiple deployments

Specify just X-Y, doesn't apply to container called X-Y

knative-metrics-robot · 2020-03-30T19:29:05Z

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	91.1%	91.5%	0.4

Cynocracy · 2020-03-30T22:17:09Z

/lgtm

knative#159)

#159) (#161)

aliok · 2020-04-03T07:28:45Z

I wanted to give this a try today.
All works fine. Precedence of deployment+containerName over containerName is also good.
Image overrides with env vars continue working fine, no regressions there.

Here are the cases I tried:

cat <<EOS |kubectl apply -f -
---
apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
  name: knative-eventing
  namespace: knative-eventing
spec:
  registry:
    override:
      eventing-controller: docker.io/foo
EOS

cat <<EOS |kubectl apply -f -
---
apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
  name: knative-eventing
  namespace: knative-eventing
spec:
  registry:
    override:
      eventing-controller/eventing-controller: docker.io/foo
EOS

cat <<EOS |kubectl apply -f -
---
apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
  name: knative-eventing
  namespace: knative-eventing
spec:
  registry:
    override:
      eventing-controller: docker.io/foo1
      eventing-controller/eventing-controller: docker.io/foo2
EOS

cat <<EOS |kubectl apply -f -
---
apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
  name: knative-eventing
  namespace: knative-eventing
spec:
  registry:
    override:
      eventing-controller: docker.io/foo1
      eventing-controller/eventing-controller: docker.io/foo2
      PING_IMAGE: docker.io/foo_ping
EOS

googlebot added the cla: yes label Mar 30, 2020

knative-prow-robot added the size/L label Mar 30, 2020

knative-prow-robot reviewed Mar 30, 2020

View reviewed changes

knative-prow-robot requested review from Cynocracy and matzew March 30, 2020 17:37

mattmoor reviewed Mar 30, 2020

View reviewed changes

knative-prow-robot added the approved label Mar 30, 2020

houshengbo force-pushed the change-container-key branch from 6081264 to 0b78105 Compare March 30, 2020 18:14

knative-prow-robot added size/XL and removed size/L labels Mar 30, 2020

mattmoor reviewed Mar 30, 2020

View reviewed changes

houshengbo force-pushed the change-container-key branch 2 times, most recently from e912d23 to 4f88185 Compare March 30, 2020 18:18

knative-prow-robot added size/XXL and removed size/XL labels Mar 30, 2020

Cynocracy reviewed Mar 30, 2020

View reviewed changes

houshengbo force-pushed the change-container-key branch from 4f88185 to 4464b88 Compare March 30, 2020 19:21

knative-prow-robot added size/L and removed size/XXL labels Mar 30, 2020

Avoid name conflict by using deployment+delimiter+container as the key

1183db3

houshengbo force-pushed the change-container-key branch from 4464b88 to 1183db3 Compare March 30, 2020 19:26

knative-prow-robot assigned Cynocracy Mar 30, 2020

knative-prow-robot added the lgtm label Mar 30, 2020

knative-prow-robot merged commit 044d9c4 into knative:master Mar 30, 2020

houshengbo pushed a commit to houshengbo/eventing-operator that referenced this pull request Mar 31, 2020

Avoid name conflict by using deployment+delimiter+container as the key (

59c49ab

knative#159)

houshengbo pushed a commit to houshengbo/eventing-operator that referenced this pull request Mar 31, 2020

Avoid name conflict by using deployment+delimiter+container as the key (

5e56c24

knative#159)

houshengbo pushed a commit that referenced this pull request Mar 31, 2020

Avoid name conflict by using deployment+delimiter+container as the key (

a19394f

#159) (#161)

This was referenced Apr 25, 2020

Eventing container names are not unique knative/eventing#2794

Closed

Image override with deployment name openshift-knative/serverless-operator#287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid name conflict by using deployment+delimiter+container as the key #159

Avoid name conflict by using deployment+delimiter+container as the key #159

houshengbo commented Mar 30, 2020 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Mar 30, 2020

mattmoor left a comment

mattmoor left a comment

mattmoor Mar 30, 2020

Cynocracy Mar 30, 2020

houshengbo Mar 30, 2020 •

edited

Loading

Cynocracy Mar 30, 2020

houshengbo Mar 30, 2020

Cynocracy Mar 30, 2020

knative-metrics-robot commented Mar 30, 2020

Cynocracy commented Mar 30, 2020

aliok commented Apr 3, 2020

Avoid name conflict by using deployment+delimiter+container as the key #159

Avoid name conflict by using deployment+delimiter+container as the key #159

Conversation

houshengbo commented Mar 30, 2020 • edited Loading

Issue to be fixed

Proposed Changes

Release Note

knative-prow-robot left a comment

Choose a reason for hiding this comment

Issue to be fixed

Proposed Changes

Release Note

knative-prow-robot commented Mar 30, 2020

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor Mar 30, 2020

Choose a reason for hiding this comment

Cynocracy Mar 30, 2020

Choose a reason for hiding this comment

houshengbo Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Cynocracy Mar 30, 2020

Choose a reason for hiding this comment

houshengbo Mar 30, 2020

Choose a reason for hiding this comment

Cynocracy Mar 30, 2020

Choose a reason for hiding this comment

knative-metrics-robot commented Mar 30, 2020

Cynocracy commented Mar 30, 2020

aliok commented Apr 3, 2020

houshengbo commented Mar 30, 2020 •

edited

Loading

houshengbo Mar 30, 2020 •

edited

Loading