Skip to content
This repository has been archived by the owner on Sep 5, 2019. It is now read-only.

Migrate to reconciler pattern for Build #446

Merged
merged 8 commits into from Oct 27, 2018

Conversation

imjasonh
Copy link
Member

Fixes #393

Proposed Changes

  • Copy code out of pkg/controller/build/ and into pkg/reconciler/build/
  • Make tests pass
  • That's pretty much it.

This is a smaller, more mechanical change than #420 which attempted to roll in a refactor which would have let us drop builder.Interface. I'd still like to get rid of that abstraction and just always interact with a kubeClient (which might be fake in tests), but I'll get to that some time after 0.2.

/assign shashwathi
/assign mattmoor

Release Note

Migrate to reconciler pattern for Build 

Copy link

@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.

@imjasonh: 0 warnings.

In response to this:

Fixes #393

Proposed Changes

  • Copy code out of pkg/controller/build/ and into pkg/reconciler/build/
  • Make tests pass
  • That's pretty much it.

This is a smaller, more mechanical change than #420 which attempted to roll in a refactor which would have let us drop builder.Interface. I'd still like to get rid of that abstraction and just always interact with a kubeClient (which might be fake in tests), but I'll get to that some time after 0.2.

/assign shashwathi
/assign mattmoor

Release Note

Migrate to reconciler pattern for Build 

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.

@shashwathi
Copy link
Contributor

I definitely like the scope of this PR. Great work @imjasonh

Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

thanks, @imjasonh! Glad to see Build picking this up!

/lgtm
/approve
/hold

Hold for one nit, if you want to address it now. If not, cancel the hold. I understand this is just the first step :)

pkg/reconciler/build/build.go Outdated Show resolved Hide resolved
pkg/reconciler/build/build.go Show resolved Hide resolved
pkg/reconciler/build/build.go Outdated Show resolved Hide resolved
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, mattmoor

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:

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

@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-build-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/build/build.go 5.6% 76.4% 70.8
pkg/reconciler/build/template_common.go Do not exist 100.0%
pkg/reconciler/build/validate_build.go Do not exist 93.0%

@imjasonh
Copy link
Member Author

/hold cancel

@mattmoor
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot merged commit e9f5b24 into knative:master Oct 27, 2018
mgencur pushed a commit to mgencur/knative-build that referenced this pull request Nov 13, 2018
* Move controller -> reconciler

* Fix tests

* update-codegen

* now with updated dep-collector

* Reverse IsDone condition

* Remove another indentation
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
* Move controller -> reconciler

* Fix tests

* update-codegen

* now with updated dep-collector

* Reverse IsDone condition

* Remove another indentation
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
* Move controller -> reconciler

* Fix tests

* update-codegen

* now with updated dep-collector

* Reverse IsDone condition

* Remove another indentation
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.

None yet

5 participants