deployed-labeler: improve error handling, add GET endpoint #86

adrienthebo · 2022-11-03T23:46:03Z

Description

deployed-labeler: bump commit limit to 5 pages, 100 results

deployed-labeler: emit error messages from /deployed endpoint

This commit generates user readable error messages when the /deployed endpoint is called without required parameters.

deployed-labeler: expose log level CLI option, adjust log levels

deployed-labeler: add GET handler for deployed to indicate label status

deployed-labeler: serialise errors as strings

per golang/go#5161 serializing an error object to json emits an empty object; this caused deployed-labeler to emit an empty object when erroring. This commit converts all errors to strings before returning to the user to provide more context.

Related Issue(s)

How to test

Test error handling

Create a malformed GitHub key
Run curl "http://localhost:8080/deployed?commit=470062367333b5d9992188dbc9ff89465c0ab1e3&team=workspace"
verify that an error is emitted and returned to the user.

Test status endpoint

Remove the deployed labels from Log JSON as strings not an array of bytes gitpod#14215
run curl "http://localhost:8080/deployed?commit=470062367333b5d9992188dbc9ff89465c0ab1e3&team=workspace"
confirm that the correct PR is listed as unlabeled

Release Notes

Documentation

This commit generates user readable error messages when the /deployed endpoint is called without required parameters.

roboquat · 2022-11-03T23:46:06Z

@adrienthebo: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

werft-gitpod-dev-com · 2022-11-03T23:46:07Z

cannot start job - please talk to whoever's in charge of your Werft installation

ArthurSens

Really nice addition of the GET endpoint! I can see that helping us a lot when debugging failures 🙂

I've made just a few suggestions about go

ArthurSens · 2022-11-04T11:47:25Z

plugins/deployed-labeler/main.go

+		var preconditionFailed struct {
+			Errors []string
 		}

+		preconditionFailed.Errors = make([]string, 0, 1)
+		preconditionFailed.Errors = append(preconditionFailed.Errors, fmt.Sprintf("team and commit are required parameters. Team: '%v', commit: '%v'", team, commitSHA))


What was your goal in creating a new type to store the error?

Could we use standard error libs? Maybe errors would make more sense here

What was your goal in creating a new type to store the error?

The PR mention's per golang/go#5161 serializing an error object to json emits an empty object; this caused deployed-labeler to emit an empty object when erroring. This commit converts all errors to strings before returning to the user to provide more context.

Seems to be serialisation and making sure the errors are returned in the HTTP response. 🤔

~~Update! New err type is not required. Rolled it back to error~~

This won't render internal gh errors i.e when a malformed token is used. We get an empty errors field which @adrienthebo was trying to fix.

Example

{ "deployedPRs": { "team": null, "all": null }, "errors": [ "non-200 OK status code: 401 Unauthorized body: \"{\\\"message\\\":\\\"Bad credentials\\\",\\\"documentation_url\\\":\\\"https://docs.github.com/graphql\\\"}\"" ] }

plugins/deployed-labeler/main.go

adrienthebo · 2022-11-04T15:16:18Z

@ArthurSens thanks for the detailed review! This PR was the result of a bunch of fairly blind hacking and stumbling around Go's error handling; a lot of decisions were based on "well, it compiles, so good enough!" I'll run through and address your comments and do another round of general cleanup and will follow up with you on another review - likely on Monday since it's Friday night your time, but perhaps tonight if you're burning the weekend oil. 😊 No rush!

kylos101 · 2022-11-30T18:09:17Z

Hi @gitpod-io/engineering-delivery-operations-experience it looks like this is caught in limbo, so I added you all back as a reviewer.

liam-j-bennett · 2022-12-01T15:58:37Z

@kylos101 @adrienthebo This work also might have gotten caught up in the changes in the team structure (i.e. Scout Team). Could this work be handed off?

kylos101 · 2022-12-02T13:41:00Z

@kylos101 @adrienthebo This work also might have gotten caught up in the changes in the team structure (i.e. Scout Team). Could this work be handed off?

Hey @liam-j-bennett 👋 we discussed with @meysholdt and @corneliusludmann during the platform/self-hosted to devx/delivery transformation, and my understanding is that Delivery Team owns the labeling component.

Is it in the list of Delivery owned components? Perhaps something changed since then? cc: @corneliusludmann

corneliusludmann · 2022-12-02T13:49:43Z

Is it in the list of Delivery owned components? Perhaps something changed since then?

No, nothing has changed. I think @liam-j-bennett's comment was about Adrien being busy with scout team work and not in the position to continue this and should be handed over to someone else from the delivery team.

I removed Adrien from the corresponding issue https://github.com/gitpod-io/ops/issues/6409 and put it back to “Scheduled” so that someone else can pick this up soon.

Pothulapati · 2022-12-05T10:49:13Z

Now that the PR compiles, Testing this PR now. 👀

Pothulapati · 2022-12-05T11:37:40Z

Update, Now that the changes work:

The move from Error to String for returning errors seem to be necessary to handle Internal API errors that Github Client returns. This is useful when there is a malfunctioning token that is passed, etc.

{
  "deployedPRs": {
    "team": null,
    "all": null
  },
  "errors": [
    "non-200 OK status code: 401 Unauthorized body: \"{\\\"message\\\":\\\"Bad credentials\\\",\\\"documentation_url\\\":\\\"https://docs.github.com/graphql\\\"}\""
  ]
}

per golang/go#5161 serializing an `error` object to json emits an empty object; this caused deployed-labeler to emit an empty object when erroring. This commit converts all errors to strings before returning to the user to provide more context. Signed-off-by: Tarun Pothulapati <[email protected]>

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati · 2022-12-05T11:56:02Z

PR is now ready to review! Cleaned up commit messages by rebasing things. 👍🏼

Pothulapati · 2022-12-06T07:24:07Z

@ArthurSens Just to confirm before proceeding, The deployment process is manual as per thereadme doc right? :)

ArthurSens · 2022-12-06T13:13:16Z

@ArthurSens Just to confirm before proceeding, The deployment process is manual as per thereadme doc right? :)

It has been a long time since I did anything here, but I believe it still the same process 😅

adrienthebo added 2 commits November 3, 2022 19:34

deployed-labeler: bump commit limit to 5 pages, 100 results

1e99553

deployed-labeler: emit error messages from /deployed endpoint

9a51525

This commit generates user readable error messages when the /deployed endpoint is called without required parameters.

adrienthebo requested a review from ArthurSens November 3, 2022 23:46

roboquat added do-not-merge/release-note-label-needed size/L labels Nov 3, 2022

ArthurSens suggested changes Nov 4, 2022

View reviewed changes

kylos101 requested a review from a team November 30, 2022 18:08

Pothulapati force-pushed the alt/deployed-labeler/error-handling branch 2 times, most recently from 2192857 to 41521d1 Compare December 5, 2022 11:20

Pothulapati self-assigned this Dec 5, 2022

Pothulapati force-pushed the alt/deployed-labeler/error-handling branch from 41521d1 to be466da Compare December 5, 2022 11:35

Pothulapati requested a review from ArthurSens December 5, 2022 11:37

Pothulapati force-pushed the alt/deployed-labeler/error-handling branch from be466da to 5a51b9a Compare December 5, 2022 11:50

adrienthebo added 4 commits December 5, 2022 11:53

deployed-labeler: add GET handler for deployed to indicate label status

fa3a71a

update readme

54fdfe8

deployed-labeler: expose log level CLI option, adjust log levels

11b2df5

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the alt/deployed-labeler/error-handling branch from 5a51b9a to 11b2df5 Compare December 5, 2022 11:55

ArthurSens approved these changes Dec 5, 2022

View reviewed changes

Pothulapati removed the do-not-merge/release-note-label-needed label Dec 7, 2022

roboquat merged commit 3bb1445 into main Dec 7, 2022

roboquat deleted the alt/deployed-labeler/error-handling branch December 7, 2022 05:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deployed-labeler: improve error handling, add GET endpoint #86

deployed-labeler: improve error handling, add GET endpoint #86

adrienthebo commented Nov 3, 2022

roboquat commented Nov 3, 2022

werft-gitpod-dev-com bot commented Nov 3, 2022

ArthurSens left a comment

ArthurSens Nov 4, 2022

Pothulapati Dec 5, 2022

Pothulapati Dec 5, 2022 •

edited

Loading

adrienthebo commented Nov 4, 2022 •

edited by werft-gitpod-dev-com bot

Loading

kylos101 commented Nov 30, 2022

liam-j-bennett commented Dec 1, 2022

kylos101 commented Dec 2, 2022

corneliusludmann commented Dec 2, 2022 •

edited

Loading

Pothulapati commented Dec 5, 2022 •

edited by werft-gitpod-dev-com bot

Loading

Pothulapati commented Dec 5, 2022

Pothulapati commented Dec 5, 2022 •

edited by werft-gitpod-dev-com bot

Loading

Pothulapati commented Dec 6, 2022

ArthurSens commented Dec 6, 2022 •

edited by werft-gitpod-dev-com bot

Loading

deployed-labeler: improve error handling, add GET endpoint #86

deployed-labeler: improve error handling, add GET endpoint #86

Conversation

adrienthebo commented Nov 3, 2022

Description

Related Issue(s)

How to test

Release Notes

Documentation

roboquat commented Nov 3, 2022

werft-gitpod-dev-com bot commented Nov 3, 2022

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens Nov 4, 2022

Choose a reason for hiding this comment

Pothulapati Dec 5, 2022

Choose a reason for hiding this comment

Pothulapati Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

adrienthebo commented Nov 4, 2022 • edited by werft-gitpod-dev-com bot Loading

kylos101 commented Nov 30, 2022

liam-j-bennett commented Dec 1, 2022

kylos101 commented Dec 2, 2022

corneliusludmann commented Dec 2, 2022 • edited Loading

Pothulapati commented Dec 5, 2022 • edited by werft-gitpod-dev-com bot Loading

Pothulapati commented Dec 5, 2022

Pothulapati commented Dec 5, 2022 • edited by werft-gitpod-dev-com bot Loading

Pothulapati commented Dec 6, 2022

ArthurSens commented Dec 6, 2022 • edited by werft-gitpod-dev-com bot Loading

Pothulapati Dec 5, 2022 •

edited

Loading

adrienthebo commented Nov 4, 2022 •

edited by werft-gitpod-dev-com bot

Loading

corneliusludmann commented Dec 2, 2022 •

edited

Loading

Pothulapati commented Dec 5, 2022 •

edited by werft-gitpod-dev-com bot

Loading

Pothulapati commented Dec 5, 2022 •

edited by werft-gitpod-dev-com bot

Loading

ArthurSens commented Dec 6, 2022 •

edited by werft-gitpod-dev-com bot

Loading