Refactor Build Status fields #536

qu1queee · 2021-01-15T15:47:18Z

This fixes #463 and #494

Note: this a follow-up based on the discussions in #463 , we are not longer introducing error codes, but rather an enhancement in the Build Status fields based on the feedback on that issue.

This should conclude a set of enhancements we have been doing in both Build/BuildRun Status fields. This PR proposes an enhancement on the Build Status fields. This will provide more clarity for users on why a validation happened and will also standardize the Status.Reason via the usage of a one-word camelCase constant. We also introduce a distinction on which type of referenced secret is missing, previously we were only highlighting if one or multiple secrets were missing, but we didn´t make any references to where in the Build definition was this secret used.

The following are examples of some of the validation scenarios on how the new fields will be populated.

All validations succeeded

spec.source secret is missing

spec.output secret is missing

multiple secrets are missing

ClusterBuildStrategy is missing

HeavyWombat

Like I wrote in the other comment, it is a tough call. At the moment, I like the idea of harmonizing the validations, because we touch the code anyway. Always harder to do it later.

pkg/controller/build/build_controller.go

vendor/modules.txt

SaschaSchwarze0

/lgtm

gabemontero

OK @qu1queee I've at least done, at least I hope, a better job of applying my idea to the precise spots in the code which are relevant.

Hopefully it is at least clearer than it was before.

I've talked with @adambkaplan and he is going to chime in on the enhancement vs. tech debt discussion point, and based on how that goes, short term implications for this change as well as perhaps and we move forward with similar changes.

Also, @otaviof agreed to take a look at this suggestion as well, to provide another voice on whether what I'm suggesting makes sense, and whether or not he agrees it is a stylistic improvement.

Then we can go from there.

thanks

pkg/controller/build/build_controller.go

adambkaplan

@qu1queee hopefully @gabemontero's follow up comments clarify what his is going after. What he is describing is a refactoring opportunity - we have several disjoint functions that validate the Build object in some form or fashion. Rather than have separate functions with their own signatures and quirks, we can use this opportunity to reduce complexity by conforming the validation functions to a shared interface.

David Copeland coined the term "slop" for code that we know is messy when we write it [1]. We could mark this a tech debt issue to address later, but experience dictates that these items are rarely, if ever, cleaned up. Better to refactor to cleaner code when the opportunity presents itself.

[1] https://naildrivin5.com/blog/2012/10/05/making-it-right-technical-debt-vs-slop.html

qu1queee · 2021-01-27T21:38:49Z

@adambkaplan please see this PR changes in detail. You are citing an article around "slop code/technical debt" that states

Writing code that does not adhere to our personal standards of correctness isn’t technical debt - it’s slop.

Code with no tests isn’t technical debt, it’s slop.
Code that passes its test due to copy and paste isn’t technical debt, it’s slop.
Code with conditional expressions that wrap the screen isn’t technical debt, it’s slop.
Code that you write, without knowing why…it’s slop.

this PR is not the above, PR speaks for itself. We are not adding these disjoint functions in this PR, these functions were already there, so I'm struggling to understand why you will quote an article around slop code, when this PR didn't write that code. I'm not against improving the existing code, please see my previous comment(and I quote myself again):

I do see the benefit on what you are saying and I would opt-in for such an enhancement, at least around:

an external file with a well define interface for validations

an approach to iterate on objects that implement this interface

I'm against not having a full understanding on the scope of this PR and trying to request a change that can easily be tackle as a follow-up, follow-up where I would be more than happy to see new authors(I certainly can spend time on such stylistic details, but more contributions are welcome). If we go with this line of thinking, then why not to address also in this PR some of the go cycle reports for the build/pkg/controller/build/build_controller.go? . I think the answer is certainly because is out of the scope.

otaviof · 2021-01-28T06:40:32Z

pkg/apis/build/v1alpha1/build_types.go

+type BuildReason string
+
+const (
+	// SucceedStatus indicates that all validations Succeeded


A nit comment, we should add a new line after each attribute, following the other attributes described in the same package.

pkg/controller/build/build_controller.go

Adding support for secrets differenciation errors in a Build Definition of one-word camelCase constants for Build validations Adding support for Status.Reason and Status.Message fields Ensure usage of Build constants to populate the Status.Reason Ensure appropiate message for the Status.Message Signed-off-by: Zoe <xigxjn@cn.ibm.com>

This add support for the new Status.Message field in Build. This refactors the previous usage of Status.Reason field, to ensure it matches the new camel-case constants. The above applies to both unit and integration tests. Signed-off-by: Zoe <xigxjn@cn.ibm.com>

Done via operator-sdk Signed-off-by: Zoe <xigxjn@cn.ibm.com>

Signed-off-by: Zoe <xigxjn@cn.ibm.com>

Introduce a table that explains the `Status.Reason` one-word validation in detail. Signed-off-by: Zoe <xigxjn@cn.ibm.com>

This introduces a new Interface that hosts a common func() to execute validations. Each different validation implements the above interface in order to trigger a validation. At the moment we have five different validations.

qu1queee · 2021-02-01T17:00:52Z

/retest

gabemontero · 2021-02-01T17:17:00Z

My input has been addressed @qu1queee - thanks !

I'll

/approve

and then leave it to the other reviewers here to lgtm

openshift-ci-robot · 2021-02-01T17:17:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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~~ [gabemontero]

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

adambkaplan

/lgtm

qu1queee added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2021

openshift-ci-robot requested review from sbose78 and zhangtbj January 15, 2021 15:47

qu1queee removed request for sbose78 and zhangtbj January 15, 2021 15:47

qu1queee assigned qu1queee and xiujuan95 and unassigned qu1queee Jan 15, 2021

qu1queee force-pushed the refactor_build_fields branch 15 times, most recently from f755f07 to f19e541 Compare January 18, 2021 15:33

qu1queee mentioned this pull request Jan 18, 2021

Add validation for build.spec.source.url #512

Merged

qu1queee force-pushed the refactor_build_fields branch 5 times, most recently from 8e813d8 to 007e96b Compare January 19, 2021 10:28

HeavyWombat reviewed Jan 26, 2021

View reviewed changes

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

vendor/modules.txt Show resolved Hide resolved

SaschaSchwarze0 approved these changes Jan 27, 2021

View reviewed changes

openshift-ci-robot assigned SaschaSchwarze0 Jan 27, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2021

gabemontero requested changes Jan 27, 2021

View reviewed changes

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

adambkaplan reviewed Jan 27, 2021

View reviewed changes

otaviof reviewed Jan 28, 2021

View reviewed changes

pkg/controller/build/build_controller.go Outdated Show resolved Hide resolved

qu1queee and others added 5 commits February 1, 2021 11:04

Update Build CRDs

7c01bc8

Done via operator-sdk Signed-off-by: Zoe <xigxjn@cn.ibm.com>

Update go modules and vendor

b5aed2f

Signed-off-by: Zoe <xigxjn@cn.ibm.com>

Add documentation for the Build Status enhancement

f60605c

Introduce a table that explains the `Status.Reason` one-word validation in detail. Signed-off-by: Zoe <xigxjn@cn.ibm.com>

qu1queee force-pushed the refactor_build_fields branch from 1725668 to f60605c Compare February 1, 2021 10:04

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2021

Add validate pkg

678e3ea

This introduces a new Interface that hosts a common func() to execute validations. Each different validation implements the above interface in order to trigger a validation. At the moment we have five different validations.

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2021

qu1queee requested review from otaviof, adambkaplan and HeavyWombat February 1, 2021 19:12

adambkaplan approved these changes Feb 1, 2021

View reviewed changes

openshift-ci-robot assigned adambkaplan Feb 1, 2021

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

openshift-merge-robot merged commit c0bfa60 into shipwright-io:master Feb 1, 2021

qu1queee deleted the refactor_build_fields branch February 2, 2021 08:12

qu1queee mentioned this pull request Feb 26, 2021

Show different types of secret missing in build error message #494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Build Status fields #536

Refactor Build Status fields #536

qu1queee commented Jan 15, 2021 •

edited

HeavyWombat left a comment

SaschaSchwarze0 left a comment

gabemontero left a comment

adambkaplan left a comment

qu1queee commented Jan 27, 2021

otaviof Jan 28, 2021

qu1queee commented Feb 1, 2021

gabemontero commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

adambkaplan left a comment

Refactor Build Status fields #536

Refactor Build Status fields #536

Conversation

qu1queee commented Jan 15, 2021 • edited

All validations succeeded

spec.source secret is missing

spec.output secret is missing

multiple secrets are missing

ClusterBuildStrategy is missing

HeavyWombat left a comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

gabemontero left a comment

Choose a reason for hiding this comment

adambkaplan left a comment

Choose a reason for hiding this comment

qu1queee commented Jan 27, 2021

otaviof Jan 28, 2021

Choose a reason for hiding this comment

qu1queee commented Feb 1, 2021

gabemontero commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

qu1queee commented Jan 15, 2021 •

edited