refactor: add missing error checks #7079

ankitm123 · 2020-04-19T17:38:58Z

Signed-off-by: ankitm123 [email protected]

Submitter checklist

Change is code complete and matches issue description.
Change is covered by existing or new tests.

Which issue this PR fixex

fixes #6851

pkg/cloud/amazon/testutils/session_test_utils.go

pkg/cmd/opts/commands.go

pkg/cloud/amazon/testutils/session_test_utils.go

pkg/cmd/opts/commands.go

jenkins-x-bot · 2020-04-19T17:39:13Z

Hi @ankitm123. Thanks for your PR.

I'm waiting for a jenkins-x 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.

hferentschik · 2020-04-20T07:29:19Z

/ok-to-test

pkg/cmd/clients/factory.go

pkg/cloud/amazon/testutils/session_test_utils.go

pkg/builds/jenkinsfiles.go

pkg/cmd/create/create_jenkins_token.go

pkg/jenkinsfile/pipeline.go

pkg/jenkins/utils.go

pkg/expose/exposecontroller.go

ankitm123 · 2020-05-08T16:11:16Z

/test lint

ankitm123 · 2020-05-08T18:26:18Z

So after rebasing, the lint failed again. So, I decided to split the linting into two files - linter.sh and linter-extra.sh, keeping the increased limits for both of those linting jobs.

ankitm123 · 2020-05-09T10:26:28Z

/test lint

hferentschik

Looking good. Not so sure between the separation between lint and lint-extra. Where is the differentiation and how would we evolve things? Were would one add a new check and why? Can you explain your motivation?

hferentschik · 2020-05-11T06:54:41Z

Makefile

@@ -213,6 +213,7 @@ importfmt: get-fmt-deps
 lint: ## Lint the code
 	./hack/gofmt.sh
 	./hack/linter.sh
+	./hack/linter-extra.sh


Why the separation? What do you try to achieve?

hferentschik · 2020-05-11T07:03:28Z

hack/linter-extra.sh

+	${DIR}/install_golint.sh -b $GOPATH/bin v1.20.0
+fi
+
+export GO111MODULE=on


Personally, I find it hard to see what the difference between lint and lint-extra is. I fear this might confuse others as well. What do we gain?

AFAICT, the bulk of the scripts are identical. So if we really want to split into multiple lint steps in Makefile and jenkins-x-lint.yaml, then I think it should be a single linter script and you pass in by argument which checks to run.

If this is just about splitting into more tasks and contexts, one thing I want to do for a long time it to break out a codegen context (`jenkins-x-codegen.yaml) and so the code generation checks separately. That's technically really not linting and it has confused people before. I'd rather do that than splitting lint and lint-extra (unless there is something I am missing here)

lint -> all the old linters, lint-extra -> new linters (ununsed and errcheck). It's a bit difficult for me to know where the script really fails. I tried various combinations, and this is the one which succeeded. I think it might make sense to coordinate over slack, if that's an option. I can try making the changes, and if you can get me the logs, that would help us debug it 🥇 wdyt? Let me try to separate the code gen from linting, and see what happens.

hferentschik · 2020-05-11T07:03:58Z

pkg/cmd/gc/gc_gke.go

+	if err != nil {
+		return errors.Wrapf(err, "checking if file %s exists", path)
+	}
+	if exists {


hferentschik · 2020-05-11T07:04:11Z

pkg/cmd/get/get_pipeline.go

@@ -123,7 +123,10 @@ func (o *GetPipelineOptions) Run() error {
 			if err != nil {
 				return err
 			}
-			o.dump(jenkins, job.Name, &table)
+			err = o.dump(jenkins, job.Name, &table)


hferentschik · 2020-05-11T07:04:35Z

pkg/pipeline_events/elasticsearch.go

@@ -90,7 +92,10 @@ func (e ElasticsearchProvider) SendRelease(r *v1.Release) error {
 				r.Namespace: r.CreationTimestamp.String(),
 			},
 		}
-		e.SendIssue(&esissue)
+		err = e.SendIssue(&esissue)
+		if err != nil {


ankitm123 · 2020-05-11T10:37:07Z

@hferentschik combining all the linters into one lint file, made it fail again. Can we increase the memory further. There is also the option to use a lower value of GOGC, this will reduce the memory usage, but at the expense of making the tests run longer. https://github.com/golangci/golangci-lint#memory-usage-of-golangci-lint
The jx code base is much bigger than the golangci-lint repo, so it makes sense why it breaks. Also in the future, we should add more linters (staticcheck, go cyclo), which will make it impossible to lint the code base imo. Wdyt? fyi, istio uses abt 31GB to lint it's codebase 😄

ankitm123 · 2020-05-11T11:09:01Z

So the lint passed after I set GOGC=10 💯 . Not sure, how to add the new code-gen to the list of existing checks ...

hferentschik

Looking good, let me enable jenkins-x-codegen context so that we can see this context run. Once it is configured we can see it works and merge. Then we will make it a required context.

hferentschik · 2020-05-11T13:55:14Z

jenkins-x-codegen.yml

@@ -0,0 +1,32 @@
+buildPack: none


we will need to enable the new context in the configuration for the repo.

Makefile

hferentschik · 2020-05-11T13:57:15Z

jenkins-x-codegen.yml

+              - name: generate
+                image: golang:1.12.10
+                command: ./hack/generate.sh
+                dir: /workspace/go/src/github.com/jenkins-x/jx


nitpick, missing newline

hferentschik · 2020-05-11T14:32:20Z

@ankitm123 - fyi https://github.com/jenkins-x/environment-tekton-weasel-dev/pull/581. Once that's merging the new context should appear and we can rebuild the whole PR. If all looks good we can merge and then we can make it a required context.

hferentschik · 2020-05-11T14:51:33Z

/test codegen

hferentschik · 2020-05-11T15:03:14Z

@ankitm123 :

Showing logs for build jenkins-x-jx-pr-7079-codegen-gvrfd-189 stage ci and container step-generate
/bin/sh: 1: cd: can't cd to /workspace/go/src/github.com/jenkins-x/**
Pipeline failed on stage 'ci' : container 'step-generate'. The execution of the pipeline has stopped.

I think we need the copy parts from the lint status check as well to move the sources to the "right" location. At least while we are still on Go 1.12.

Signed-off-by: ankitm123 <[email protected]>

ankitm123 · 2020-05-11T15:44:13Z

@hferentschik great, the codegen bit works now, gonna remove that copy part from the lint (it's not needed any more imo).

abayer · 2020-05-11T16:05:36Z

/retest

hferentschik · 2020-05-11T16:51:31Z

jenkins-x-codegen.yml

+                    cpu: 3
+                    memory: 6Gi
+            steps:
+              - name: mk-jx-project-dir


hferentschik · 2020-05-11T16:52:45Z

/lgtm

jenkins-x-bot · 2020-05-11T16:53:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hferentschik

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

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

cagiti · 2020-05-14T12:23:05Z

This is a great change, but I think it would be useful to log an error for missing error checks in one PR before returning the error, because it alters behaviour of the codebase (albeit incorrect). Then a subsequent PR which returns the error would be required (when the impact is understood), and arguably could be a case for a minor version increment.

ankitm123 · 2020-05-15T09:51:44Z

This is a great change, but I think it would be useful to log an error for missing error checks in one PR before returning the error, because it alters behaviour of the codebase (albeit incorrect_). Then a subsequent PR which returns the error would be required (when the impact is understood), and arguably could be a case for a minor version increment.

You are right. Honestly, I thought that this PR might lead to some cases, where the downstream code relies on no errors being returned. There are about 90 places in the codebase where I have skipped errchecks (using nolint). Not catching the error can lead to unintended results though, for instance we still dont handle errors for any of the defer functions and I am not sure what kind of bugs they have introduced (for instance we dont know if those files are being closed properly). Are you seeing any places where handling the errors have led to some issues? Would be nice to re-evaluate the logic there.

jenkins-x-bot added the do-not-merge/work-in-progress label Apr 19, 2020

houndci-bot reviewed Apr 19, 2020

View reviewed changes

pkg/cloud/amazon/testutils/session_test_utils.go Outdated Show resolved Hide resolved

pkg/cloud/amazon/testutils/session_test_utils.go Outdated Show resolved Hide resolved

pkg/cloud/amazon/testutils/session_test_utils.go Outdated Show resolved Hide resolved

jenkins-x-bot requested review from jstrachan and rajdavies April 19, 2020 17:39

houndci-bot reviewed Apr 19, 2020

View reviewed changes

pkg/cmd/opts/commands.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 19, 2020

View reviewed changes

pkg/cmd/opts/commands.go Outdated Show resolved Hide resolved

jenkins-x-bot added needs-ok-to-test size/L labels Apr 19, 2020

ankitm123 force-pushed the jx-6851 branch from a7cd0a8 to 213128b Compare April 19, 2020 18:51

hferentschik changed the title ~~[WIP] refactor: add missing error checks~~ [wip] refactor: add missing error checks Apr 20, 2020

jenkins-x-bot added ok-to-test and removed needs-ok-to-test labels Apr 20, 2020

ankitm123 force-pushed the jx-6851 branch from 213128b to 992bd04 Compare April 20, 2020 23:47

jenkins-x-bot added size/XXL and removed size/L labels Apr 20, 2020

hferentschik suggested changes Apr 23, 2020

View reviewed changes

pkg/cmd/clients/factory.go Outdated Show resolved Hide resolved

ankitm123 force-pushed the jx-6851 branch from 992bd04 to fc04909 Compare April 29, 2020 21:46

jenkins-x-bot added size/L and removed size/XXL labels Apr 29, 2020

houndci-bot reviewed Apr 29, 2020

View reviewed changes

pkg/cloud/amazon/testutils/session_test_utils.go Outdated Show resolved Hide resolved

pkg/builds/jenkinsfiles.go Outdated Show resolved Hide resolved

ankitm123 force-pushed the jx-6851 branch from fc04909 to 38f04f5 Compare April 30, 2020 02:15

houndci-bot reviewed Apr 30, 2020

View reviewed changes

pkg/cmd/create/create_jenkins_token.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 30, 2020

View reviewed changes

pkg/cmd/create/create_jenkins_token.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 30, 2020

View reviewed changes

pkg/cmd/create/create_jenkins_token.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 30, 2020

View reviewed changes

pkg/jenkinsfile/pipeline.go Outdated Show resolved Hide resolved

pkg/jenkins/utils.go Outdated Show resolved Hide resolved

ankitm123 force-pushed the jx-6851 branch from 38f04f5 to d3525e9 Compare May 1, 2020 00:39

houndci-bot reviewed May 1, 2020

View reviewed changes

pkg/expose/exposecontroller.go Outdated Show resolved Hide resolved

ankitm123 force-pushed the jx-6851 branch from da1f71d to b792f9f Compare May 8, 2020 16:53

ankitm123 force-pushed the jx-6851 branch from b792f9f to b57e2d1 Compare May 9, 2020 10:13

hferentschik reviewed May 11, 2020

View reviewed changes

ankitm123 force-pushed the jx-6851 branch from b57e2d1 to 23a3a34 Compare May 11, 2020 10:23

ankitm123 force-pushed the jx-6851 branch from 23a3a34 to d019ee1 Compare May 11, 2020 10:45

ankitm123 force-pushed the jx-6851 branch from d019ee1 to ae397cc Compare May 11, 2020 12:00

hferentschik reviewed May 11, 2020

View reviewed changes

ankitm123 force-pushed the jx-6851 branch from ae397cc to 24f5979 Compare May 11, 2020 14:14

ankitm123 force-pushed the jx-6851 branch from 24f5979 to 268344c Compare May 11, 2020 15:12

refactor: add missing error checks

9117193

Signed-off-by: ankitm123 <[email protected]>

ankitm123 force-pushed the jx-6851 branch from 268344c to 9117193 Compare May 11, 2020 15:39

hferentschik approved these changes May 11, 2020

View reviewed changes

jenkins-x-codegen.yml

cpu: 3

memory: 6Gi

steps:

- name: mk-jx-project-dir

Copy link

hferentschik May 11, 2020

Choose a reason for hiding this comment

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

:+1

jenkins-x-bot assigned hferentschik May 11, 2020

jenkins-x-bot added the lgtm label May 11, 2020

jenkins-x-bot added the approved label May 11, 2020

jenkins-x-bot merged commit 22ce213 into jenkins-x:master May 11, 2020

refactor: add missing error checks #7079

refactor: add missing error checks #7079

Uh oh!

Conversation

ankitm123 commented Apr 19, 2020

Submitter checklist

Which issue this PR fixex

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jenkins-x-bot commented Apr 19, 2020

Uh oh!

hferentschik commented Apr 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankitm123 commented May 8, 2020

Uh oh!

ankitm123 commented May 8, 2020

Uh oh!

ankitm123 commented May 9, 2020

Uh oh!

hferentschik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitm123 commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankitm123 commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hferentschik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hferentschik commented May 11, 2020

Uh oh!

hferentschik commented May 11, 2020

Uh oh!

hferentschik commented May 11, 2020

Uh oh!

ankitm123 commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abayer commented May 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitm123 commented May 11, 2020 •

edited

Loading

ankitm123 commented May 11, 2020 •

edited

Loading

ankitm123 commented May 11, 2020 •

edited

Loading

cagiti commented May 14, 2020 •

edited

Loading