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.

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

Restructure buildrun code into separate resources #595

Conversation

qu1queee
Copy link
Contributor

@qu1queee qu1queee commented Feb 17, 2021

Changes

This is a rebase on top of #593 . This PR is doing the following:

  • [1] Separate controller/reconcile logic. This ensures that between controller definition and reconcile logic, we have a clear separation.
  • [2] Refactor buildRun reconciler class. Move some functions into new or existing classes under the internal resource package. This class was one of the most hardest files to read. So the goal here is to make it easier for future contributors.
  • [3] Ensure proper code coverage. Because I externalize a lot of the functions in [2], into the internal resource pkg, I´m adding a bunch of new unit-tests to increase the coverage of those functions.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Release notes block has been filled in, or marked NONE

Release Notes

Restructure BuildRun reconcile code into separate classes.

@qu1queee qu1queee added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2021
@openshift-ci-robot openshift-ci-robot added release-note Label for when a PR has specified a release note and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 17, 2021
@qu1queee qu1queee added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2021
@qu1queee qu1queee force-pushed the qu1queee/organize_br_reconcile branch 8 times, most recently from 4483825 to 30b0e19 Compare February 18, 2021 15:14
This ensures that between controller definition
and reconcile logic, we have a clear separation.

This also add a missing suite for the resources internal pkg.
Move some functions into new or existing classes under
the internal resource package.
@qu1queee qu1queee force-pushed the qu1queee/organize_br_reconcile branch 2 times, most recently from 5a5888c to 362d0b1 Compare February 18, 2021 15:23
@qu1queee qu1queee removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2021
@qu1queee qu1queee requested review from SaschaSchwarze0 and HeavyWombat and removed request for xiujuan95 February 18, 2021 15:27
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Nice, we get even smaller source files. :-) Some suggestions.

pkg/reconciler/buildrun/resources/conditions.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/strategies.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/strategies.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/build.go Outdated Show resolved Hide resolved
@qu1queee qu1queee force-pushed the qu1queee/organize_br_reconcile branch from 38bc0a8 to 015745d Compare February 22, 2021 14:47
This add new unit-tests to ensure a decent code coverage
for all the different classes under the resource pkg of
buildrun.

Signed-off-by: Sascha Schwarze <[email protected]>
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2021
@openshift-merge-robot openshift-merge-robot merged commit 0803afd into shipwright-io:master Feb 22, 2021
@qu1queee qu1queee deleted the qu1queee/organize_br_reconcile branch February 22, 2021 15:45
@gabemontero gabemontero added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants