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.

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

Consolidate use of errors #4389

Merged
merged 1 commit into from
Jun 4, 2024
Merged

Consolidate use of errors #4389

merged 1 commit into from
Jun 4, 2024

Conversation

jcaamano
Copy link
Contributor

@jcaamano jcaamano commented May 23, 2024

As follows:

  • Replaced all equivalent uses of github.com/pkg/errors for standard library errors
  • Replaced the uses of github.com/pkg/errors Wrapf for fmt.Errorf. The need for the stack trace that Wrapf brings seems arbitrary
  • Replaced the uses of the standard library errors Join, as well as k8s aggregate errors, with our own implementation which works similarly to standard library's but formats the concatenation as the k8s aggregate errors separating with commas rather than new lines which is an inconvenience. When compared to k8s aggregate errors, our implementation is simpler catering to our needs and complies fully with current wrapping mechanism whereas the former does not support 'errors.As'

@jcaamano jcaamano requested a review from a team as a code owner May 23, 2024 15:55
@jcaamano jcaamano requested a review from girishmg May 23, 2024 15:55
@jcaamano jcaamano force-pushed the join-error branch 3 times, most recently from 2b36008 to 0569859 Compare May 23, 2024 16:12
As follows:

* Replaced all equivalent uses of github.com/pkg/errors for standard library
  errors
* Replaced the uses of github.com/pkg/errors Wrapf for fmt.Errorf. The need for
  the stack trace that Wrapf brings seems arbitrary
* Replaced the uses of the standard library errors Join, as well as k8s
  aggregate errors, with our own implementation which works similarly to standard
  library's but formats the concatenation as the k8s aggregate errors separating
  with commas rather than new lines which is an inconvenience. When compared to
  k8s aggregate errors, our implementation is simpler catering to our needs and
  complies fully with current wrapping mechanism whereas the former does not
  support 'errors.As'

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
@tssurya tssurya added the area/code-cleanup Issues related to code clean ups, FUPs, refactors, addressing review comments in FUPs label May 27, 2024
Copy link
Contributor

@flavio-fernandes flavio-fernandes left a comment

Choose a reason for hiding this comment

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

nice!
/lgtm

@@ -83,22 +81,22 @@ func getOvsPortBandwidth(ifname string, dir direction) (int64, error) {
func getInterfaceIngressBandwith(ifname string) (int64, error) {
qos_id, err := ovsGet("port", ifname, "qos", "")
if err != nil {
return 0, errors.Wrapf(err, "Failed to get qos for port %s", ifname)
return 0, fmt.Errorf("failed to get qos for port %s: %w", ifname, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@girishmg girishmg merged commit 8ce0b0c into ovn-org:master Jun 4, 2024
35 checks passed
@tssurya
Copy link
Member

tssurya commented Jun 6, 2024

I think this was a really nice cleanup, however unless we add some section here: https://ovn-kubernetes.io/developer-guide/developer/ for developers I get this feeling in no time we might go back to using different things in different spots. Just putting what's in the commit message under a sub heading is good enough for the docs, that way next time we can just point people to Best Practices or Coding Style for OVN-Kubernetes etc.. will be nice to have a section there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/code-cleanup Issues related to code clean ups, FUPs, refactors, addressing review comments in FUPs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants