Skip to content

Add mechanism to communicate back to Bitballoon from Build Plugins #735

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

Closed
DavidWells opened this issue Feb 6, 2020 · 14 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@DavidWells
Copy link
Contributor

We need a way to communicate back to bitballoon from build plugins.

If an error occurs, it would be great if we can surface those issues to users like described in #711 (comment)

Additionally plugin outputs & other data would likely need to pipe back into the deploy UI of app.netlify.com to enhance the experience. You an imagine this structured information being shown on the deploy summary for example:

image


So we need some sort of mechanism to pass back errors / stack traces when build fail.

Additionally we need a mechanism that will allow plugins to communicate back and display additional information (in deploy summary or elsewhere depending on where design lands)

Perhaps something like:

function netlifyPlugin(config) {
  return {
    name: 'my-plugin-one',
    onInit: async (pluginApi) => {
      pluginApi.utils.report({
        message: 'Thing XYZ happened'
      })
    },
  }
}

However it materializes, we should keep in mind how this can & should evolve.

Related #161

@DavidWells DavidWells added the enhancement New feature or request label Feb 6, 2020
@ehmicky
Copy link
Contributor

ehmicky commented Feb 10, 2020

Yes I agree.

At the moment plugins can do this by throwing an error but as @jlengstorf points out in this comment (thumbed up by one of our users @talves), this does not allow differentiating between bugs/crashes and assertion/validation errors.

The syntax chosen by @jlengstorf was utils.build.failWithError('message'). Yours is utils.report({ message: 'message' }).

I think this question is also related to cancelling builds (#241), which are basically the same thing with the additional requirement that the error also marks the build as "cancelled": utils.cancelBuild('message').

We could merge those 3 different proposals into a single API:

// Stops plugin but does not stop build
utils.build.error('message')
// Stops plugin and stops build
utils.build.terminate('message')
// Same but also mark the build as cancelled in the UI 
utils.build.cancel('message')

If we ever need to expand those methods, we can add a options object optional parameter.

What do you think?

This was referenced Feb 10, 2020
@jlengstorf
Copy link
Contributor

I like where you're going, @ehmicky — I think I'd push for more verbose names, though, to make it easier to infer what will happen

perhaps something like this?

// stops plugin but does not stop build
utils.build.recoverableError('message')
// or
utils.build.logErrorAndContinue('message')

// stops both
utils.build.failBuild('message')
// or
utils.build.logErrorAndTerminate('message')

// cancel
utils.build.cancelBuild('message')
// or
utils.build.logErrorAndCancel('message')

.error gives me the impression that it will cause a full failure, so I think a longer method name with more clarity could help here

@ehmicky
Copy link
Contributor

ehmicky commented Feb 11, 2020

Yes this makes sense. What about using your first examples but dropping the build namespace?

// stops plugin but does not stop build
utils.recoverableError('message')
// stops both
utils.failBuild('message')
// cancel
utils.cancelBuild('message')

@erquhart
Copy link
Contributor

I think it makes sense to have a single util/namespace. Let's keep in mind that this API can be added to or superseded without having to break things. We're going to learn a lot over the beta and we'll be able to make better informed design decisions in the future.

For now, let's go with:

utils.build.logError()
utils.build.failBuild()
utils.build.cancelBuild()

No one expects logging an error to fail anything, and the other two are pretty self explanatory.

@erquhart erquhart added this to the build-plugins/beta milestone Feb 13, 2020
@ehmicky
Copy link
Contributor

ehmicky commented Feb 13, 2020

Sure this makes sense.
Small naming thing: could we remove the repeated Build? It's already in the namespace build.

utils.build.logError()
utils.build.fail()
utils.build.cancel()

@erquhart
Copy link
Contributor

erquhart commented Feb 13, 2020

@vbrown608 @mraerino this is the API we're looking at for minimal communication from plugins back to the API, are there related tickets we should link for backend support?

(Added Marcus as he provided related feedback to @DavidWells.)

@vbrown608
Copy link

vbrown608 commented Feb 13, 2020

Looks like this is just related to access for cancellation and logging. Access to cancellation was added by https://github.com/netlify/bitballoon/pull/5467. Still need to complete https://github.com/netlify/buildbot/issues/583 (make buildbot consume that token and pass it to netlify-builds). I'm starting that today.

In terms of logging, we're discussing that here: https://github.com/netlify/bitballoon/issues/5468 - basically buildbot should send logs directly to firebase rather than going through bitballon

@ehmicky
Copy link
Contributor

ehmicky commented Feb 24, 2020

Does utils.build.logError() interrupt the current function (like throwing an exception) or does it continue to the next statement?

It seems like the name logError() would imply the second one. However, considering this function will probably be used to indicate validation errors, the first behavior would probably make more sense. In which case, we might consider renaming it?

@DavidWells
Copy link
Contributor Author

This is confusing to me as well.

What is the use case of utils.build.logError() and we are we adding it?

@ehmicky
Copy link
Contributor

ehmicky commented Feb 25, 2020

I think the original idea from @jlengstorf was for recoverable errors, i.e. when the plugin should fail, but not the whole build.

However the current name logError() make it sound like it would log the message and go to the next statement (like console.log()). console.log() or console.error() currently work for that use case.

I suggest renaming logError to make it clearer. What about this:

  • utils.build.logError() -> utils.build.failPlugin()
  • utils.build.fail() -> utils.build.failBuild()
  • utils.build.cancel() -> utils.build.cancelBuild()

Also I think calling the namespace build is a little confusing. What's common with all those methods is error reporting. One of them is actually plugin-wise not build-wise. So I suggest renaming the namespace to error which would give:

  • utils.error.failPlugin()
  • utils.error.failBuild()
  • utils.error.cancelBuild()

What do you think?

@ehmicky
Copy link
Contributor

ehmicky commented Feb 25, 2020

Current status update of this feature:

What's left:

@erquhart
Copy link
Contributor

@ehmicky I know we made progress here, can you provide a quick status update?

@ehmicky
Copy link
Contributor

ehmicky commented Mar 26, 2020

The utils.build is now complete. We currently have two pending items on the UI / front-end side:

  • The error type (e.g. "Canceled") should be shown in the UI instead of "Failed" (see this comment)
  • The error information should be added to the Deploy summary in the UI (see this comment)

@erquhart
Copy link
Contributor

Nice!! If the utilities work is complete, we can close this and use #711 to drive requirements for surfacing errors in the UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants