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

add message to '.Error()' output for LsError #623

Merged
merged 2 commits into from
Oct 26, 2022

Conversation

Diaphteiros
Copy link
Contributor

How to categorize this PR?

/area logging
/kind enhancement
/priority 3

What this PR does / why we need it:
When wrapping an error in an object of type LsError, a message can be specified. Calling .Error() on the LsError object returns only the original error message, though. If this LsError object is then again wrapped, neither .Message, nor .Error() called on the new object will return the message specified during the first wrapping. This results in a loss of potentially interesting information.

To fix this, the .Error() implementation of LsError has been enhanced in the LsError type: Instead of just returning .Error() of the internal error, it will now return a combination of .Message and .Error(). It also checks if former one already contains the latter one, to avoid duplicate errors.

Release note:

Calling `.Error()` on a `LsError` object will now return a combination of the LsError's message and its internal error, instead of just returning the internal error's message.

@Diaphteiros Diaphteiros requested a review from a team as a code owner October 20, 2022 12:23
@gardener-robot gardener-robot added area/logging Logging related kind/enhancement Enhancement, improvement, extension priority/3 Priority (lower number equals higher priority) needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Oct 20, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 20, 2022
@Diaphteiros
Copy link
Contributor Author

cc @reshnm

@gardener-robot
Copy link

@In-Ko You have pull request review open invite, please check

@Diaphteiros Diaphteiros added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 25, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 25, 2022
@Diaphteiros
Copy link
Contributor Author

/test

@testmachinery
Copy link

testmachinery bot commented Oct 25, 2022

Testrun: e2e-njlmt
Workflow: e2e-njlmt-wf
Phase: Failed

+--------------------+--------------------+-----------+----------+
|        NAME        |        STEP        |   PHASE   | DURATION |
+--------------------+--------------------+-----------+----------+
| create-cluster     | create-cluster     | Succeeded | 3m11s    |
| create-registry    | create-registry    | Succeeded | 2m41s    |
| install-landscaper | install-landscaper | Succeeded | 44s      |
| integration-test   | test               | Failed    | 6m4s     |
| delete-cluster     | delete-cluster     | Omitted   | 0s       |
| delete-registry    | delete-registry    | Omitted   | 0s       |
+--------------------+--------------------+-----------+----------+

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 25, 2022
Copy link
Contributor

@achimweigel achimweigel left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Oct 26, 2022
@Diaphteiros Diaphteiros merged commit 817d43e into gardener:master Oct 26, 2022
@Diaphteiros Diaphteiros deleted the enhance-errors branch October 26, 2022 06:47
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logging Logging related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) priority/3 Priority (lower number equals higher priority) reviewed/lgtm Has approval for merging size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants