Closes #3262: Modify unnecessarily exported methods to unexported #3289

kobayashi · 2020-09-28T04:47:57Z

Changes

This is for issue #3262.

Only ConvertErrorf is kept as it is in v1beta1 because the function is imported in v1alpha1.

$ grep -r ConvertErrorf *pkg/apis/pipeline/v1alpha1/conversion_error.go:39:var convertErrorf = v1beta1.ConvertErrorf
pkg/apis/pipeline/v1beta1/conversion_error.go:45:// ConvertErrorf creates a CannotConvertError from the field name and format string.
pkg/apis/pipeline/v1beta1/conversion_error.go:46:func ConvertErrorf(field, msg string, args ...interface{}) error {
$

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

linux-foundation-easycla · 2020-09-28T04:48:00Z

The committers are authorized under a signed CLA.

✅ kobayashi (76449a7)

tekton-robot · 2020-09-28T04:48:11Z

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

vdemeester

Overall seems ok. I am a bit worried on pkg/git being used elsewhere (as a package) but it should be fine.

/ok-to-test

vdemeester · 2020-09-28T09:06:33Z

pkg/contexts/contexts.go

+// withDefaultConfigurationName notes on the context for nested validation
 // that there is a default configuration name, which affects how an empty
 // configurationName is validated.
-func WithDefaultConfigurationName(ctx context.Context) context.Context {
+func withDefaultConfigurationName(ctx context.Context) context.Context {
 	return context.WithValue(ctx, hdcnKey{}, struct{}{})
 }

-// HasDefaultConfigurationName checks to see whether the given context has
+// hasDefaultConfigurationName checks to see whether the given context has
 // been marked as having a default configurationName.
-func HasDefaultConfigurationName(ctx context.Context) bool {
+func hasDefaultConfigurationName(ctx context.Context) bool {
 	return ctx.Value(hdcnKey{}) != nil
 }


If those are not exported, this means they are not used. We could remove them entirely 😉

Thank you for the review. I removed these functions, also removed emitEvent emitErrorEvent from pkg/reconciler/events/event.go which are unused.

vdemeester · 2020-09-28T09:10:30Z

/kind cleanup

tekton-robot · 2020-09-28T22:23:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/events/event.go	86.4%	95.0%	8.6

tekton-robot · 2020-09-28T22:37:22Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/events/event.go	86.4%	95.0%	8.6

afrittoli · 2020-10-01T13:16:27Z

pkg/reconciler/events/event.go

@@ -102,22 +102,3 @@ func EmitError(c record.EventRecorder, err error, object runtime.Object) {
 		c.Event(object, corev1.EventTypeWarning, EventReasonError, err.Error())
 	}
 }
-
-// EmitEvent emits an event for object if afterCondition is different from beforeCondition


These were added because of backward compatibility, but since we're cleaning up all unnecessarily exported methods, I'd say it makes sense to remove them.

afrittoli

Thanks for this!
/lgtm

vdemeester

/lgtm
/cc @imjasonh

tekton-robot · 2020-11-08T04:58:41Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/events/event.go	87.0%	95.2%	8.3

kobayashi · 2020-11-08T04:59:15Z

Just rebased to the latest. Please let me know anything I can do to proceed this PR.

vdemeester

/lgtm

kobayashi · 2021-02-02T07:29:12Z

Hi, I just make sure if I still need some changes.

imjasonh · 2021-02-02T12:09:48Z

Hi, I just make sure if I still need some changes.

The PR needs to be rebased on master, but other than that the change still lgtm, so we should be able to get it merged after you rebase.

tekton-robot · 2021-02-18T05:15:12Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/events/event.go	87.0%	95.2%	8.3

kobayashi · 2021-02-18T06:38:52Z

Thank you!
Just rebased my PR repo.

kobayashi · 2021-02-23T07:21:00Z

@imjasonh , could you review this again?

imjasonh · 2021-02-23T07:22:47Z

/lgtm

Sorry for the delay, and thanks for doing this!

vdemeester · 2021-02-24T12:49:08Z

/lgtm cancel
Needs a rebase, cancelling the lgtm to unblock the merge queue 🙏🏼

vdemeester · 2021-02-24T16:33:07Z

(interesting.. it doesn't show anymore..)
/lgtm

afrittoli · 2021-02-24T22:36:13Z

I'm dropping lgmt to unlock tide as this needs a rebase

afrittoli · 2021-02-24T22:36:20Z

/lgtm cancel

vdemeester · 2021-02-25T06:03:04Z

@afrittoli interesting.. when in the merge loop (all checks ok), it shows the rebase message from github, when not, it doesn't show it anymore.. 😓 Putting a hold for now while we wait for a rebase.
/hold

@kobayashi can you rebase your PR against master ?

kobayashi · 2021-03-01T07:06:08Z

Thank you, @vdemeester .
Rebased it again to master.

tekton-robot · 2021-03-01T07:07:18Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/events/event.go	87.0%	95.2%	8.3

kobayashi · 2021-03-01T16:07:27Z

/retest

vdemeester · 2021-03-01T16:44:00Z

/hold cancel
/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 28, 2020

tekton-robot requested review from afrittoli and dibyom September 28, 2020 04:48

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 28, 2020

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 28, 2020

vdemeester reviewed Sep 28, 2020

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 Sep 28, 2020

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 28, 2020

kobayashi force-pushed the 3262-exported-methods branch from d7f5fa5 to e6a9bb9 Compare September 28, 2020 22:35

afrittoli reviewed Oct 1, 2020

View reviewed changes

tekton-robot assigned afrittoli Oct 1, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2020

vdemeester reviewed Oct 1, 2020

View reviewed changes

tekton-robot requested a review from imjasonh October 1, 2020 13:31

tekton-robot assigned vdemeester Oct 1, 2020

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2020

vdemeester mentioned this pull request Oct 5, 2020

Cleaning up multiple unnecessarily exported methods #3327

Closed

4 tasks

kobayashi force-pushed the 3262-exported-methods branch from e6a9bb9 to 1bc032a Compare November 8, 2020 04:56

tekton-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 8, 2020

vdemeester reviewed Nov 9, 2020

View reviewed changes

tekton-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 18, 2021

tekton-robot assigned imjasonh Feb 23, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2021

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2021

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2021

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2021

kobayashi added 3 commits March 1, 2021 01:58

Modify unnecessarily exported methods to unexported

a2d0593

Rmove unused functions

9e57474

Unexport additional functions

8ce7312

kobayashi force-pushed the 3262-exported-methods branch from c2e9e6d to 8ce7312 Compare March 1, 2021 07:04

tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 1, 2021

tekton-robot merged commit 10865b8 into tektoncd:master Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #3262: Modify unnecessarily exported methods to unexported #3289

Closes #3262: Modify unnecessarily exported methods to unexported #3289

kobayashi commented Sep 28, 2020 •

edited

Loading

linux-foundation-easycla bot commented Sep 28, 2020 •

edited

Loading

tekton-robot commented Sep 28, 2020

vdemeester left a comment

vdemeester Sep 28, 2020

kobayashi Sep 28, 2020

vdemeester commented Sep 28, 2020

tekton-robot commented Sep 28, 2020

tekton-robot commented Sep 28, 2020

afrittoli Oct 1, 2020

afrittoli left a comment

vdemeester left a comment

tekton-robot commented Nov 8, 2020

kobayashi commented Nov 8, 2020

vdemeester left a comment

kobayashi commented Feb 2, 2021

imjasonh commented Feb 2, 2021

tekton-robot commented Feb 18, 2021

kobayashi commented Feb 18, 2021

kobayashi commented Feb 23, 2021

imjasonh commented Feb 23, 2021

vdemeester commented Feb 24, 2021

vdemeester commented Feb 24, 2021

afrittoli commented Feb 24, 2021

afrittoli commented Feb 24, 2021

vdemeester commented Feb 25, 2021

kobayashi commented Mar 1, 2021

tekton-robot commented Mar 1, 2021

kobayashi commented Mar 1, 2021

vdemeester commented Mar 1, 2021

Closes #3262: Modify unnecessarily exported methods to unexported #3289

Closes #3262: Modify unnecessarily exported methods to unexported #3289

Conversation

kobayashi commented Sep 28, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

linux-foundation-easycla bot commented Sep 28, 2020 • edited Loading

tekton-robot commented Sep 28, 2020

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Sep 28, 2020

Choose a reason for hiding this comment

kobayashi Sep 28, 2020

Choose a reason for hiding this comment

vdemeester commented Sep 28, 2020

tekton-robot commented Sep 28, 2020

tekton-robot commented Sep 28, 2020

afrittoli Oct 1, 2020

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Nov 8, 2020

kobayashi commented Nov 8, 2020

vdemeester left a comment

Choose a reason for hiding this comment

kobayashi commented Feb 2, 2021

imjasonh commented Feb 2, 2021

tekton-robot commented Feb 18, 2021

kobayashi commented Feb 18, 2021

kobayashi commented Feb 23, 2021

imjasonh commented Feb 23, 2021

vdemeester commented Feb 24, 2021

vdemeester commented Feb 24, 2021

afrittoli commented Feb 24, 2021

afrittoli commented Feb 24, 2021

vdemeester commented Feb 25, 2021

kobayashi commented Mar 1, 2021

tekton-robot commented Mar 1, 2021

kobayashi commented Mar 1, 2021

vdemeester commented Mar 1, 2021

kobayashi commented Sep 28, 2020 •

edited

Loading

linux-foundation-easycla bot commented Sep 28, 2020 •

edited

Loading