Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove all duplicate transformer code #26

Closed
jcrossley3 opened this issue Apr 21, 2020 · 3 comments · Fixed by #29
Closed

Remove all duplicate transformer code #26

jcrossley3 opened this issue Apr 21, 2020 · 3 comments · Fixed by #29
Assignees
@jcrossley3

Comments

@jcrossley3
Copy link
Contributor

We don't want to maintain two separate code branches for the fields common to both KnativeServing and KnativeEventing. All of these files should be removed:

pkg/reconciler/knativeeventing/common/config_maps.go
pkg/reconciler/knativeeventing/common/config_maps_test.go
pkg/reconciler/knativeserving/common/config_maps.go

pkg/reconciler/knativeeventing/common/images.go
pkg/reconciler/knativeeventing/common/images_test.go
pkg/reconciler/knativeserving/common/images.go
pkg/reconciler/knativeserving/common/images_test.go

Their consolidated versions should be relocated, but I'm not sure where. How about pkg/transformers?
@jcrossley3
Copy link
Contributor Author

/assign

@houshengbo
Copy link
Contributor

houshengbo commented Apr 22, 2020 •
edited
Loading

Will they be used somewhere else other than in the code under pkg/reconciler?
Does pkg/reconciler/transformers work?

@jcrossley3
Copy link
Contributor Author

Does pkg/reconciler/transformers work?

Sure. Do you like that better than pkg/reconciler/common? (just in case we have shared stuff other than transformers)

jcrossley3 added a commit to jcrossley3/operator that referenced this issue Apr 22, 2020
@jcrossley3
Consolidate transformers for common CRD fields, registry and config
d83df59 
Fixes knative#26

Also factored the common test utilities out of images.go and into a
common/testing package.

All tests should remain intact -- eventing had a few more than serving
-- now all in a single place.
@jcrossley3 jcrossley3 mentioned this issue Apr 22, 2020
Merged
jcrossley3 added a commit to jcrossley3/operator that referenced this issue Apr 27, 2020
@jcrossley3
Consolidate transformers for common CRD fields, registry and config
d8c45e6 
Fixes knative#26

Also factored the common test utilities out of images.go and into a
common/testing package.

All tests should remain intact -- eventing had a few more than serving
-- now all in a single place.
knative-prow-robot pushed a commit that referenced this issue Apr 28, 2020
@jcrossley3
Consolidate transformers for common CRD fields, registry and config (#29
7bd06d0 
)

* Consolidate transformers for common CRD fields, registry and config

Fixes #26

Also factored the common test utilities out of images.go and into a
common/testing package.

All tests should remain intact -- eventing had a few more than serving
-- now all in a single place.

* Refactored extensions and made component transformers more explicit

I like seeing the exact list of transformers in the reconciler's
transform function.
matzew added a commit to matzew/knative-operator that referenced this issue Jul 29, 2020
@matzew
Add openshift hacks 016 (knative#26)
8f59a1d 
* 🐳 dockerfile for image build

Signed-off-by: Matthias Wessendorf <[email protected]>

* 💄 making OWNERs right for Openshift org

Signed-off-by: Matthias Wessendorf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jcrossley3 jcrossley3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Consolidate transformers for common CRD fields, registry and config jcrossley3/operator
2 participants
@jcrossley3 @houshengbo