Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[kubeadm/app/]switch to github.com/pkg/errors #70271

Merged
merged 2 commits into from
Oct 30, 2018
Merged

[kubeadm/app/]switch to github.com/pkg/errors #70271

merged 2 commits into from
Oct 30, 2018

Conversation

yuexiao-wang
Copy link
Contributor

Signed-off-by: yuexiao-wang wang.yuexiao@zte.com.cn

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
switch to github.com/pkg/errors the packages as follows:
cmd/kubeadm/app/cmd/upgrade
cmd/kubeadm/app/cmd/util
cmd/kubeadm/app/discovery
cmd/kubeadm/app/discovery/file
cmd/kubeadm/app/phases/addons/dns
cmd/kubeadm/app/phases/addons/proxy
cmd/kubeadm/app/phases/bootstraptoken/clusterinfo/
cmd/kubeadm/app/phases/etcd

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # Fixes # kubernetes/kubeadm#1183

Special notes for your reviewer:
@fabriziopandini @neolit123

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 26, 2018
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 26, 2018
neolit123
neolit123 approved these changes Oct 26, 2018
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm
/kind cleanup

@yuexiao-wang thanks, might have been better to change all kubeadm packages in a single PR.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 26, 2018
@neolit123
Copy link
Member

/ok-to-test
/kind cleanup

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 26, 2018
@yuexiao-wang
Copy link
Contributor Author

/test pull-kubernetes-kubemark-e2e-gce-big

@yuexiao-wang
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-100-performance

fabriziopandini
fabriziopandini suggested changes Oct 26, 2018
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@yuexiao-wang thanks!
Only one nit and then ok for approval

cmd/kubeadm/app/cmd/upgrade/apply.go Outdated
@@ -250,13 +251,13 @@ func EnforceVersionPolicies(flags *applyFlags, versionGetter upgrade.VersionGett
if versionSkewErrs != nil {

if len(versionSkewErrs.Mandatory) > 0 {
return fmt.Errorf("The --version argument is invalid due to these fatal errors:\n\n%v\nPlease fix the misalignments highlighted above and try upgrading again", kubeadmutil.FormatErrMsg(versionSkewErrs.Mandatory))
return fmt.Errorf("the --version argument is invalid due to these fatal errors:\n\n%v\nPlease fix the misalignments highlighted above and try upgrading again", kubeadmutil.FormatErrMsg(versionSkewErrs.Mandatory))
Copy link
Member

Choose a reason for hiding this comment

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

Errors.new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

cmd/kubeadm/app/cmd/upgrade/apply.go Outdated
}

if len(versionSkewErrs.Skippable) > 0 {
// Return the error if the user hasn't specified the --force flag
if !flags.force {
return fmt.Errorf("The --version argument is invalid due to these errors:\n\n%v\nCan be bypassed if you pass the --force flag", kubeadmutil.FormatErrMsg(versionSkewErrs.Skippable))
return fmt.Errorf("the --version argument is invalid due to these errors:\n\n%v\nCan be bypassed if you pass the --force flag", kubeadmutil.FormatErrMsg(versionSkewErrs.Skippable))
Copy link
Member

Choose a reason for hiding this comment

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

errors.Errorf
(Same applies to all fmt.Errorf remaining in the packages in scope)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Got it and I need to amend these applications to my PR.
There are so many fmt.Errorf in kubeadm packages. so I only switch to errors.Wrap and error.New in my PR.

@joshuarubin joshuarubin mentioned this pull request Oct 26, 2018
Closed
@yuexiao-wang
Copy link
Contributor Author

@neolit123 Thanks for your review.
It has so many fmt.Errorf in kubeadm packages, and I'm afraid that it is a super large PR if I only submit a single PR. According to @fabriziopandini comments, I will amend this PR to apply error.Errorf and submit them to this PR.

@fabriziopandini
Copy link
Member

@yuexiao-wang if you keep fixednthe number of packages in scope and change only the fmt.Errorf statement the size of the PR will remain acceptable, but I case thinks goes out of control you can always split the PR in two

@yuexiao-wang
Copy link
Contributor Author

yuexiao-wang commented Oct 27, 2018 •
edited

@fabriziopandini I find about 400 occurrences about fmt.Errorf in the rest of kubeadm package.
I have two questions:

  1. we should switch to errors.Errorf when return fmt.Errorf in xxx.go and xxx_test.go, right?
  2. did't switch errors.Errorf in these cases as follows:
    a) kubeadmutil.CheckErr(fmt.Errorf("failed to write the new configuration to the file %q: %v", newCfgPath, err))
    b) notReadyDaemonSets = append(notReadyDaemonSets, fmt.Errorf("DaemonSet %q not healthy: %v", dsName, err))

@fabriziopandini
Copy link
Member

@yuexiao-wang
You are doing a great job.
Small, well focused PR usually get faster review. but considering that in this case the change is really simple IMO we can relax a bit on the "small" side. Nevertheless if you are worried about the number of changes you can split the PR into smaller PRs. Up to you.

  1. if I got it right, yes please switch from return fmt.Errorf to return errors.New or return errors.Wrap or return errors.Errorf
  2. IMO you can switch to errors.Wrap in both cases as well

@neolit123
Copy link
Member

there is also https://godoc.org/github.com/pkg/errors#Wrapf if needed in places.

@yuexiao-wang
Copy link
Contributor Author

@neolit123 i am puzzled about errors.Errorf and errors.Wrapf and do you any suggestion? thanks

@yuexiao-wang
Copy link
Contributor Author

yuexiao-wang commented Oct 27, 2018 •
edited

@fabriziopandini thanks. i will try it

@neolit123
Copy link
Member

@yuexiao-wang

b) notReadyDaemonSets = append(notReadyDaemonSets, fmt.Errorf("DaemonSet %q not healthy: %v", dsName, err))

return fmt.Errorf("DaemonSet %q not healthy: %v", dsName, err)

can be changed to:

return errors.Wrapf(err, "DaemonSet %q not healthy", dsName)

so we should use Wrapf() too if needed.
otherwise you have to do:

return errors.Wrap(err, fmt.Sprintf("DaemonSet %q not healthy", dsName))

does that make sense?

@yuexiao-wang yuexiao-wang force-pushed the switch-errors-1 branch 4 times, most recently from 9f65b1d to ad5efbd Compare October 28, 2018 14:18
@yuexiao-wang
Copy link
Contributor Author

CI will be green.
@fabriziopandini @neolit123 PTAL

neolit123
neolit123 reviewed Oct 29, 2018
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

@yuexiao-wang thanks a lot for this big cleanup! awesome.
added some minor comments.

@kubernetes/sig-cluster-lifecycle-pr-reviews
ideally, we need more reviewers on this PR.

cmd/kubeadm/app/phases/certs/certs.go Outdated
@@ -52,7 +54,7 @@ func CreatePKIAssets(cfg *kubeadmapi.InitConfiguration) error {
}

if err := certTree.CreateTree(cfg); err != nil {
return fmt.Errorf("Error creating PKI assets: %v", err)
return errors.Wrap(err, "Error creating PKI assets")
Copy link
Member

Choose a reason for hiding this comment

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

please make this error string start with lowecase "error" instead of "Error"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks
I will fix it and check all the cleanup in this PR again

cmd/kubeadm/app/phases/certs/pkiutil/pki_helpers.go Outdated
@@ -38,12 +39,12 @@ import (
func NewCertificateAuthority(config *certutil.Config) (*x509.Certificate, *rsa.PrivateKey, error) {
key, err := certutil.NewPrivateKey()
if err != nil {
return nil, nil, fmt.Errorf("unable to create private key [%v]", err)
return nil, nil, errors.Wrap(err, "unable to create private key [%v]")
Copy link
Member

Choose a reason for hiding this comment

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

please remove [%v]

[1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

cmd/kubeadm/app/phases/certs/pkiutil/pki_helpers.go Outdated
}

cert, err := certutil.NewSelfSignedCACert(*config, key)
if err != nil {
return nil, nil, fmt.Errorf("unable to create self-signed certificate [%v]", err)
return nil, nil, errors.Wrap(err, "unable to create self-signed certificate [%v]")
Copy link
Member

Choose a reason for hiding this comment

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

[1]

cmd/kubeadm/app/phases/certs/pkiutil/pki_helpers.go Outdated
@@ -53,12 +54,12 @@ func NewCertificateAuthority(config *certutil.Config) (*x509.Certificate, *rsa.P
func NewCertAndKey(caCert *x509.Certificate, caKey *rsa.PrivateKey, config *certutil.Config) (*x509.Certificate, *rsa.PrivateKey, error) {
key, err := certutil.NewPrivateKey()
if err != nil {
return nil, nil, fmt.Errorf("unable to create private key [%v]", err)
return nil, nil, errors.Wrap(err, "unable to create private key [%v]")
Copy link
Member

Choose a reason for hiding this comment

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

[1]

cmd/kubeadm/app/phases/certs/pkiutil/pki_helpers.go Outdated
}

cert, err := certutil.NewSignedCert(*config, key, caCert, caKey)
if err != nil {
return nil, nil, fmt.Errorf("unable to sign certificate [%v]", err)
return nil, nil, errors.Wrap(err, "unable to sign certificate [%v]")
Copy link
Member

Choose a reason for hiding this comment

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

[1]

cmd/kubeadm/app/phases/certs/pkiutil/pki_helpers.go Outdated
}

certificatePath := pathForCert(pkiPath, name)
if err := certutil.WriteCert(certificatePath, certutil.EncodeCertPEM(cert)); err != nil {
return fmt.Errorf("unable to write certificate to file %q: [%v]", certificatePath, err)
return errors.Errorf(certificatePath, err, "unable to write certificate to file %q: [%v]")
Copy link
Member

Choose a reason for hiding this comment

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

should be:

errors.Wrapf(err, "unable to write certificate to file %q", certificatePath)

also optionally you can change %q to %s if you want this to be consistent with the errors in the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

cmd/kubeadm/app/phases/upgrade/policy.go Outdated
@@ -52,32 +53,32 @@ func EnforceVersionPolicies(versionGetter VersionGetter, newK8sVersionStr string
clusterVersionStr, clusterVersion, err := versionGetter.ClusterVersion()
if err != nil {
// This case can't be forced: kubeadm has to be able to lookup cluster version for upgrades to work
skewErrors.Mandatory = append(skewErrors.Mandatory, fmt.Errorf("Unable to fetch cluster version: %v", err))
skewErrors.Mandatory = append(skewErrors.Mandatory, errors.Wrap(err, "Unable to fetch cluster version: %v"))
Copy link
Member

Choose a reason for hiding this comment

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

please remove : %v at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

cmd/kubeadm/app/util/marshal.go Outdated
continue
}
knownKinds[typeMetaInfo.Kind] = true

// Build a GroupVersionKind object from the deserialized TypeMeta object
gv, err := schema.ParseGroupVersion(typeMetaInfo.APIVersion)
if err != nil {
errs = append(errs, fmt.Errorf("unable to parse apiVersion: %v", err))
errs = append(errs, pkgerrors.Wrap(err, "unable to parse apiVersion: %v"))
Copy link
Member

Choose a reason for hiding this comment

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

release remove : %v

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

cmd/kubeadm/app/util/system/kernel_validator.go Outdated
@@ -218,14 +220,14 @@ func (k *KernelValidator) getKernelConfigReader() (io.Reader, error) {
// config module and check again.
output, err := exec.Command(modprobeCmd, configsModule).CombinedOutput()
if err != nil {
return nil, fmt.Errorf("unable to load kernel module %q: output - %q, err - %v",
configsModule, output, err)
return nil, pkgerrors.Wrapf(err, "unable to load kernel module %q: output - %q, err - ",
Copy link
Member

Choose a reason for hiding this comment

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

lets change the end to output: %q, err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i want to change it to unable to load kernel module: %q, output: %q, err

@yuexiao-wang yuexiao-wang force-pushed the switch-errors-1 branch 3 times, most recently from 895f95e to 1a7536f Compare October 30, 2018 06:55
@fabriziopandini
Copy link
Member

/retest

@yuexiao-wang
Copy link
Contributor Author

I update the PR and split to two PR, which is convenient to review

@fabriziopandini
Copy link
Member

@yuexiao-wang if this is ready I will try to get this merged today, otherwise please let me known

@yuexiao-wang
Copy link
Contributor Author

yuexiao-wang commented Oct 30, 2018 •
edited

@fabriziopandini I think it will be ready after CI is green.

@yuexiao-wang yuexiao-wang force-pushed the switch-errors-1 branch 2 times, most recently from 826e0ae to c0a9b4d Compare October 30, 2018 08:16
@yuexiao-wang
[kubeadm/app/]switch to github.com/pkg/errors
cc303c8 
Signed-off-by: yuexiao-wang <wang.yuexiao@zte.com.cn>
@yuexiao-wang
add BUILD
c0a9b4d 
Signed-off-by: yuexiao-wang <wang.yuexiao@zte.com.cn>
@fabriziopandini
Copy link
Member

@yuexiao-wang if you need help feel free to contact me on slack.
For the error in pull-kubernetes-verify hack/verify-gofmt.sh and hack/update-gofmt.sh can help you

@yuexiao-wang
Copy link
Contributor Author

yuexiao-wang commented Oct 30, 2018 •
edited

@fabriziopandini Thanks. I have run hack/verify-gofmt.sh and hack/update-gofmt.sh before submit my PR everytime. There are no errors happen in local, which wrong args is exist in code, but it only failed in master. I don't know why

@fabriziopandini
Copy link
Member

@yuexiao-wang In that case I suggest to rebase your PR

@fabriziopandini
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, yuexiao-wang

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2018
@k8s-ci-robot k8s-ci-robot merged commit 03a145d into kubernetes:master Oct 30, 2018
@yuexiao-wang yuexiao-wang deleted the switch-errors-1 branch October 30, 2018 10:51
@poothia poothia mentioned this pull request Nov 1, 2018
Closed
@Rajat-0 Rajat-0 mentioned this pull request Nov 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 approved these changes

@fabriziopandini fabriziopandini fabriziopandini requested changes

@dixudx dixudx Awaiting requested review from dixudx

Assignees

@neolit123 neolit123

@fabriziopandini fabriziopandini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yuexiao-wang @neolit123 @fabriziopandini @k8s-ci-robot