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

feat(plugin): Add error augmentor plugin #118

Merged
merged 20 commits into from
Feb 12, 2022

Conversation

JesseTatasciore
Copy link
Member

Wire up SetupHook that will pass properties to their respective plugins

Add an error augmentor plugin that will parse bazel output and provide help
Error Augmentor

@JesseTatasciore JesseTatasciore self-assigned this Jan 19, 2022
@JesseTatasciore JesseTatasciore linked an issue Jan 19, 2022 that may be closed by this pull request
@JesseTatasciore JesseTatasciore force-pushed the jtatasciore/augment-error-message-content branch from cb61d1e to 3c5ba10 Compare January 19, 2022 19:38
examples/error-augmentor/README.md Outdated Show resolved Hide resolved
examples/error-augmentor/.aspectplugins Outdated Show resolved Hide resolved
pkg/plugin/sdk/v1alpha2/plugin/grpc.go Outdated Show resolved Hide resolved
pkg/plugin/sdk/v1alpha2/plugin/interface.go Outdated Show resolved Hide resolved
pkg/plugin/sdk/v1alpha2/proto/plugin.proto Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Show resolved Hide resolved
plugins/error-augmentor/plugin.go Show resolved Hide resolved
cmd/aspect/main.go Outdated Show resolved Hide resolved
pkg/plugin/sdk/v1alpha2/plugin/interface.go Outdated Show resolved Hide resolved
pkg/plugin/system/aspectplugins.go Outdated Show resolved Hide resolved
pkg/plugin/system/aspectplugins.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Show resolved Hide resolved
plugins/error-augmentor/plugin.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Outdated Show resolved Hide resolved
plugins/error-augmentor/plugin.go Show resolved Hide resolved
@JesseTatasciore JesseTatasciore force-pushed the jtatasciore/augment-error-message-content branch from d7b26f8 to a9861d0 Compare February 4, 2022 15:47
pkg/plugin/system/system.go Outdated Show resolved Hide resolved
pkg/plugin/system/system.go Outdated Show resolved Hide resolved
pkg/plugin/system/system.go Outdated Show resolved Hide resolved
@@ -14,6 +14,7 @@ import (
// Plugin determines how an aspect Plugin should be implemented.
type Plugin interface {
BEPEventCallback(event *buildeventstream.BuildEvent) error
Setup(properties []byte) error
Copy link
Member

@jbedard jbedard Feb 4, 2022

Choose a reason for hiding this comment

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

What do the bytes represent? Is it weird having bytes in the public API? Or is this not public?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is public. I have been thinking about this since yesterday, and I could be convinced to change the API here to take a map[string]interface{}. The disadvantage is that we limit the users from parsing the bytes straight into a typed struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree that its a little weird to have []byte on a public API like this. However it is the most convenient form for that data to be in. Makes it super easy for the plugin to parse it into its expected type. If we pass it through as something else then most plugins are just going to need to unmarshal it again creating extra work on their end

Copy link
Member Author

Choose a reason for hiding this comment

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

@f0rmiga @jbedard What do we want to do here? Do we want to switch it now? Can we merge this PR and discuss it further afterwards? Would like to get this PR in because its been open for a long time. I would be happy to make a ticket / followup PR if we want to change it out.

@f0rmiga f0rmiga merged commit 21f330a into main Feb 12, 2022
@f0rmiga f0rmiga deleted the jtatasciore/augment-error-message-content branch February 12, 2022 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Augment error message content
3 participants