Fix capitalization inconsistency in reconciler #7318

mlschmithorst · 2023-10-31T18:06:33Z

Changes

Go style states that error messages typically shouldn't be capitalized unless in logs, tests, API response, or UI.
Updates error messages returned in pkg/reconciler to begin with lowercase. Updates error logs in pkg/reconciler to begin with uppercase.

Fixes the first task in #7266

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

/kind cleanup

linux-foundation-easycla · 2023-10-31T18:06:37Z

The committers listed above are authorized under a signed CLA.

✅ login: mlschmithorst / name: Matthew Schmithorst (673ae62)

tekton-robot · 2023-10-31T18:06:43Z

Hi @mlschmithorst. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

khrm

/ok-to-test

tekton-robot · 2023-10-31T18:15:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	98.3%	99.2%	0.8

chmouel · 2023-10-31T18:15:24Z

I think there is a golangci-lint linter for that we could probably enable

mlschmithorst · 2023-10-31T19:08:52Z

I think there is a golangci-lint linter for that we could probably enable

I did see in the Development guidelines that the linter would be run with each PR, but I don't see it running currently in the list of checks. Should there be a separate issue to implement that? This is my first PR so I'm still learning how everything works.

JeromeJu · 2023-11-01T14:59:20Z

Thanks @mlschmithorst
/assign

JeromeJu · 2023-11-01T15:06:25Z

pkg/reconciler/taskrun/taskrun_test.go

  sideCars:
  retriesStatus:
  - conditions:
    - reason: TaskRunValidationFailed
      status: "False"
      type: "Succeeded"
-      message: "Provided results don't match declared results; may be invalid JSON or missing result declaration:  \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\""
+      message: "provided results don't match declared results; may be invalid JSON or missing result declaration:  \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\""


This might actually be the ones we want to keep capitalized since users would just read through the retriesStatus message.

This error message is returned from here. Is this only returned for retriesStatus or is that just for this test case? I can update the error message and the test case to be capitalized again. I just wanted to make sure this wasn't also returned somewhere else where the capitalization could be incorrect style.

Thanks for looking into this, Matthew! Your question makes sense.
The message: "Provided results...." we are seeing is user-facing.

The issue seems to be that we haven't wrapped the error being returned well enough with a capitalized error message in taskrun.go.

So the workaround for this PR here might be just to restore this change and in the future we might want to keep the error messages wrapped so such issue wouldn't recur:/

Thank you for the clarification Jerome. I have reverted that change.

JeromeJu · 2023-11-01T15:11:36Z

I think there is a golangci-lint linter for that we could probably enable

Are we referring to https://golangci-lint.run/usage/linters/#godot . Looks like it has been disabled in our repo

pipeline/.golangci.yml

Line 67 in f975869

- godot

.

chmouel · 2023-11-01T15:52:37Z

I think there is a golangci-lint linter for that we could probably enable

Are we referring to golangci-lint.run/usage/linters/#godot . Looks like it has been disabled in our repo bendory/tekton-pipelines@841c8dc/.golangci.yml#L67.

I don't see in the docs but it's stylecheck for me:

https://golangci-lint.run/usage/linters/#stylecheck

one one of my repo where i have it enabled it would show me as error if capitalized:

% make lint
Linting go files...
pkg/webhook/reconciler.go:69:10: ST1005: error strings should not be capitalized (stylecheck)
		return fmt.Errorf("Secret %q is missing %q key", ac.secretName, certresources.CACert)
		      ^
make: *** [Makefile:75: lint-go] Error 1

Fix the first task in tektoncd#7266 Update error messages returned in pkg/reconciler to begin with lowercase. Update error logs in pkg/reconciler to begin with uppercase.

JeromeJu

Thanks @mlschmithorst

JeromeJu · 2023-11-02T16:26:45Z

I think there is a golangci-lint linter for that we could probably enable

Are we referring to golangci-lint.run/usage/linters/#godot . Looks like it has been disabled in our repo bendory/tekton-pipelines@841c8dc/.golangci.yml#L67.

I don't see in the docs but it's stylecheck for me:

https://golangci-lint.run/usage/linters/#stylecheck

one one of my repo where i have it enabled it would show me as error if capitalized:
% make lint
Linting go files...
pkg/webhook/reconciler.go:69:10: ST1005: error strings should not be capitalized (stylecheck)
		return fmt.Errorf("Secret %q is missing %q key", ac.secretName, certresources.CACert)
		      ^
make: *** [Makefile:75: lint-go] Error 1

Cool, thanks @chmouel . I added this to the Epic as an improvement to be done. Will verify there which linter fits in the current use case.

JeromeJu · 2023-11-02T16:27:30Z

Thanks for the first contribution to Tekton and welcome!

/meow

tekton-robot · 2023-11-02T16:27:33Z

@JeromeJu:

In response to this:

Thanks for the first contribution to Tekton and welcome!

/meow

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.

mlschmithorst · 2023-11-02T16:51:35Z

/assign @vdemeester

vdemeester · 2023-11-06T10:04:26Z

I tend to be in the "all errors shouldn't be capitablized" side of things 🙃
/approve

tekton-robot · 2023-11-06T10:04:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeromeJu, vdemeester

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

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

JeromeJu · 2023-11-06T14:20:39Z

I tend to be in the "all errors shouldn't be capitablized" side of things 🙃 /approve

+1
Plus maybe we should also have as many "wrapped" errors as we can? according to https://go.dev/blog/go1.13-errors.

QuanZhang-William

Thanks @mlschmithorst!

/lgtm

tekton-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesnt merit a release note. labels Oct 31, 2023

tekton-robot requested review from afrittoli and jerop October 31, 2023 18:06

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 31, 2023

khrm reviewed Oct 31, 2023

View reviewed changes

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 31, 2023

tekton-robot assigned JeromeJu Nov 1, 2023

JeromeJu reviewed Nov 1, 2023

View reviewed changes

JeromeJu mentioned this pull request Nov 1, 2023

Epic: User-facing Error Handling Improvement 🧹 #7276

Closed

Fix capitalization inconsistency in reconciler

673ae62

Fix the first task in tektoncd#7266 Update error messages returned in pkg/reconciler to begin with lowercase. Update error logs in pkg/reconciler to begin with uppercase.

mlschmithorst force-pushed the error-message-capitalization branch from 7039282 to 673ae62 Compare November 2, 2023 14:19

JeromeJu approved these changes Nov 2, 2023

View reviewed changes

tekton-robot assigned vdemeester Nov 2, 2023

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2023

jerop added this to the Pipelines v0.54 milestone Nov 9, 2023

QuanZhang-William reviewed Nov 9, 2023

View reviewed changes

tekton-robot assigned QuanZhang-William Nov 9, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2023

tekton-robot merged commit e570f36 into tektoncd:main Nov 9, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix capitalization inconsistency in reconciler #7318

Fix capitalization inconsistency in reconciler #7318

mlschmithorst commented Oct 31, 2023

linux-foundation-easycla bot commented Oct 31, 2023 •

edited

Loading

tekton-robot commented Oct 31, 2023

khrm left a comment

tekton-robot commented Oct 31, 2023

chmouel commented Oct 31, 2023

mlschmithorst commented Oct 31, 2023

JeromeJu commented Nov 1, 2023

JeromeJu Nov 1, 2023

mlschmithorst Nov 1, 2023

JeromeJu Nov 1, 2023

mlschmithorst Nov 2, 2023

JeromeJu commented Nov 1, 2023 •

edited

Loading

chmouel commented Nov 1, 2023

JeromeJu left a comment

JeromeJu commented Nov 2, 2023

JeromeJu commented Nov 2, 2023

tekton-robot commented Nov 2, 2023

mlschmithorst commented Nov 2, 2023

vdemeester commented Nov 6, 2023

tekton-robot commented Nov 6, 2023

JeromeJu commented Nov 6, 2023

QuanZhang-William left a comment

Fix capitalization inconsistency in reconciler #7318

Fix capitalization inconsistency in reconciler #7318

Conversation

mlschmithorst commented Oct 31, 2023

Changes

Submitter Checklist

Release Notes

linux-foundation-easycla bot commented Oct 31, 2023 • edited Loading

tekton-robot commented Oct 31, 2023

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 31, 2023

chmouel commented Oct 31, 2023

mlschmithorst commented Oct 31, 2023

JeromeJu commented Nov 1, 2023

JeromeJu Nov 1, 2023

Choose a reason for hiding this comment

mlschmithorst Nov 1, 2023

Choose a reason for hiding this comment

JeromeJu Nov 1, 2023

Choose a reason for hiding this comment

mlschmithorst Nov 2, 2023

Choose a reason for hiding this comment

JeromeJu commented Nov 1, 2023 • edited Loading

chmouel commented Nov 1, 2023

JeromeJu left a comment

Choose a reason for hiding this comment

JeromeJu commented Nov 2, 2023

JeromeJu commented Nov 2, 2023

tekton-robot commented Nov 2, 2023

mlschmithorst commented Nov 2, 2023

vdemeester commented Nov 6, 2023

tekton-robot commented Nov 6, 2023

JeromeJu commented Nov 6, 2023

QuanZhang-William left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Oct 31, 2023 •

edited

Loading

JeromeJu commented Nov 1, 2023 •

edited

Loading