-
Notifications
You must be signed in to change notification settings - Fork 289
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
Keptn plugin added #1160
Keptn plugin added #1160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a code review, testing this plugin now 👍
internal/source/keptn/client.go
Outdated
// Client Keptn client | ||
type Client struct { | ||
// API refers to Keptn client. https://github.com/keptn/go-utils | ||
API *api.APISet | ||
} | ||
|
||
type GetEventsRequest struct { | ||
Project string | ||
FromTime time.Time | ||
} | ||
|
||
type Event struct { | ||
ID string | ||
Source string | ||
Type string | ||
Data Data | ||
} | ||
|
||
type Data struct { | ||
Message string | ||
Project string | ||
Service string | ||
Status string | ||
Stage string | ||
Result string | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments to the structures. Thanks!
internal/source/keptn/client.go
Outdated
data := Data{} | ||
err := ev.DataAs(&data) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pleasae wrap the error
log := loggerx.New(cfg.Log) | ||
exitOnError(err, log) | ||
|
||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should exit the for loop in case ctx.Done()
. Please update this. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, last small comments 👍
Co-authored-by: Pawel Kosiec <[email protected]>
Co-authored-by: Pawel Kosiec <[email protected]>
Co-authored-by: Pawel Kosiec <[email protected]>
Description
Changes proposed in this pull request:
Testing
You can refer to this article for testing. In that article I am explaining also howto implement plugin, but you don't need to implement it at all, only test.
Related issue(s)
#1089