PowerVS: Remove deprecated errors package #7544

hamzy · 2023-09-29T18:01:33Z

Wrote a program [1] to remove all occurances of the go-lang errors package.

https://github.com/hamzy/powervs-hack/tree/main/remove-errors

hamzy · 2023-09-29T20:50:22Z

/retest

hamzy · 2023-09-29T23:05:49Z

/assign @r4f4

r4f4

Since you're changing the case-ness of some output messages, don't forget to update the unit tests that check for those messages.

r4f4 · 2023-09-30T09:40:32Z

pkg/asset/installconfig/powervs/validation.go

@@ -2,7 +2,6 @@ package powervs

 import (
 	"context"
-	"errors"


This is the built-in errors lib, you don't need to remove this one.

If I leave it in, then I get:

pkg/asset/installconfig/powervs/validation.go:5:2: "errors" imported and not used

And I would think that if I had used it in that file, then the compiler would complain if I didn't include it...

r4f4 · 2023-09-30T09:42:11Z

pkg/types/powervs/validation/platform_test.go

@@ -2,7 +2,7 @@ package validation

 import (
 	"crypto/rand"
-	"errors"


No need to remove.

This one, I put back and it compiled fine.

But lint complains:

pkg/types/powervs/validation/platform_test.go:5:2: "errors" imported but not used (typecheck) "errors"

Yes because you changed the line that used it below: errors.New(...). I was saying that those errors.New calls don't necessarily need to be replaced because they are using the built-in errors lib, not the archived github.com/pkg/errors package one.

hamzy · 2023-09-30T13:45:09Z

Since you're changing the case-ness of some output messages, don't forget to update the unit tests that check for those messages.

Good catch! Updated...

openshift-merge-robot · 2023-10-03T01:21:49Z

@hamzy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-images	`a88ea28`	link	true	`/test okd-images`
ci/prow/images	`a88ea28`	link	true	`/test images`
ci/prow/okd-scos-images	`a88ea28`	link	true	`/test okd-scos-images`
ci/prow/e2e-aws-custom-security-groups	`a88ea28`	link	false	`/test e2e-aws-custom-security-groups`
ci/prow/e2e-aws-ovn	`a88ea28`	link	true	`/test e2e-aws-ovn`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

r4f4

/approve

openshift-ci · 2023-10-03T07:35:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r4f4

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

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

barbacbd · 2023-10-03T12:45:15Z

pkg/asset/installconfig/powervs/metadata.go

 	if vpcName == "" || vpcSubnet == "" {
 		return "", false, nil
 	}

 	vpc, err := m.client.GetVPCByName(ctx, vpcName)
 	if err != nil {
-		return "", false, errors.Wrap(err, "failed to get VPC")
+		return colonPercentW, false, fmt.Errorf("failed to get VPC: %w", err)


The usage here seems to have changed. This may need to be a separate commit for tracking purposes, as this doesn't look like it is only a change to remove the errors package as indicated in the PR. Same for all of the usages in this function.

The usage here changed because of a new lint error. :(

@hamzy I don't see the PR history complaining about that being an empty string. I see the linter complaining about it since it was used multiple times. My concern was that you are returning something other than the empty string. Do you have an example output of the first complaint?

pkg/asset/installconfig/powervs/metadata.go:161:10: string `: %w` has 4 occurrences, make it a constant (goconst)

https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_installer/7544/pull-ci-openshift-installer-master-golint/1708920013651447808/build-log.txt

Ah I think I see what you are asking about... let me investigate...

@barbacbd I pushed a new version...

mjturek · 2023-10-03T14:00:07Z

/lgtm

hamzy · 2023-10-03T14:04:12Z

/hold

Wrote a program [1] to remove all occurances of the go-lang errors package. 1) https://github.com/hamzy/powervs-hack/tree/main/remove-errors

mjturek · 2023-10-04T17:20:59Z

/lgtm

barbacbd · 2023-10-04T18:01:52Z

/hold cancel

barbacbd · 2023-10-04T18:03:48Z

/retest-required

openshift-ci-robot · 2023-10-04T22:29:49Z

/retest-required

Remaining retests: 0 against base HEAD 732271d and 2 for PR HEAD df5bb10 in total

hamzy · 2023-10-05T03:03:49Z

/retest-required

openshift-ci-robot · 2023-10-05T11:57:05Z

/retest-required

Remaining retests: 0 against base HEAD 3a738d8 and 1 for PR HEAD df5bb10 in total

hamzy · 2023-10-05T20:01:24Z

/retest-required

openshift-ci-robot · 2023-10-06T04:34:28Z

/retest-required

Remaining retests: 0 against base HEAD 3c3f41c and 0 for PR HEAD df5bb10 in total

openshift-ci-robot · 2023-10-06T07:07:43Z

/hold

Revision df5bb10 was retested 3 times: holding

hamzy · 2023-10-06T10:57:37Z

/retest-required
/hold cancel

openshift-ci-robot · 2023-10-06T18:07:17Z

/retest-required

Remaining retests: 0 against base HEAD 3c3f41c and 2 for PR HEAD df5bb10 in total

openshift-ci · 2023-10-06T20:51:05Z

@hamzy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-custom-security-groups	`df5bb10`	link	false	`/test e2e-aws-custom-security-groups`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

hamzy · 2023-10-06T20:52:43Z

/retest-required

openshift-ci bot requested review from barbacbd and Prashanth684 September 29, 2023 18:03

hamzy force-pushed the PowerVS-remove-error-package branch from c4e124d to 94d83a8 Compare September 29, 2023 18:37

hamzy force-pushed the PowerVS-remove-error-package branch 2 times, most recently from 157a8b4 to a22d0b7 Compare September 29, 2023 22:08

openshift-ci bot assigned r4f4 Sep 29, 2023

r4f4 reviewed Sep 30, 2023

View reviewed changes

hamzy force-pushed the PowerVS-remove-error-package branch 6 times, most recently from 4133d7c to a88ea28 Compare October 2, 2023 21:30

r4f4 reviewed Oct 3, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2023

barbacbd reviewed Oct 3, 2023

View reviewed changes

hamzy force-pushed the PowerVS-remove-error-package branch from a88ea28 to bbc6b96 Compare October 3, 2023 13:31

openshift-ci bot assigned mjturek Oct 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2023

PowerVS: Remove deprecated errors package

df5bb10

Wrote a program [1] to remove all occurances of the go-lang errors package. 1) https://github.com/hamzy/powervs-hack/tree/main/remove-errors

hamzy force-pushed the PowerVS-remove-error-package branch from bbc6b96 to df5bb10 Compare October 3, 2023 17:45

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2023

openshift-ci bot merged commit b6e9f0b into openshift:master Oct 6, 2023
21 of 22 checks passed

r4f4 mentioned this pull request Oct 11, 2023

CORS-2836: Refactor Stages, encapsulate terraform #7488

Merged

PowerVS: Remove deprecated errors package #7544

PowerVS: Remove deprecated errors package #7544

Conversation

hamzy commented Sep 29, 2023

hamzy commented Sep 29, 2023

hamzy commented Sep 29, 2023

r4f4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamzy commented Sep 30, 2023

openshift-merge-robot commented Oct 3, 2023

r4f4 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjturek commented Oct 3, 2023

hamzy commented Oct 3, 2023

mjturek commented Oct 4, 2023

barbacbd commented Oct 4, 2023

barbacbd commented Oct 4, 2023

openshift-ci-robot commented Oct 4, 2023

hamzy commented Oct 5, 2023

openshift-ci-robot commented Oct 5, 2023

hamzy commented Oct 5, 2023

openshift-ci-robot commented Oct 6, 2023

openshift-ci-robot commented Oct 6, 2023

hamzy commented Oct 6, 2023

openshift-ci-robot commented Oct 6, 2023

openshift-ci bot commented Oct 6, 2023 • edited

hamzy commented Oct 6, 2023

openshift-ci bot commented Oct 6, 2023 •

edited