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 preBuild hook for addons (Resolves #2410) #2411

Merged

Conversation

brittanydionigi
Copy link
Contributor

This creates a preBuild hook for add-ons, based on the work that was done for creating a postBuild hook. #1215

As noted in #2410, an example use case for this feature might be when there are file checks or manipulations you need to make prior to the build process.

buildResults = 'build results';
});

it('allows addons to add promises prebuild', function() {
Copy link
Member

Choose a reason for hiding this comment

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

prebuild -> preBuild

@brittanydionigi
Copy link
Contributor Author

Not sure why this is failing at AppVeyor, tests pass locally for me. Seems there are a couple PRs with similar AppVeyor issues, but not sure what the common thread could be.

@rwjblue
Copy link
Member

rwjblue commented Oct 30, 2014

Not sure why this is failing at AppVeyor

AppVeyor has not yet been green. We are slowly attempting to fix AppVeyor issues, but we generally use Travis status for mergability at this point.

@brittanydionigi
Copy link
Contributor Author

Oh good to know, thanks!

@rwjblue
Copy link
Member

rwjblue commented Oct 31, 2014

👍 from me

@rwjblue
Copy link
Member

rwjblue commented Oct 31, 2014

@stefanpenner will need to confirm

.then(this.processBuildResult.bind(this))
.then(this.addonsPostBuild.bind(this));
.then(this.processAddonBuildSteps.bind(this, 'postBuild'));
Copy link
Contributor

Choose a reason for hiding this comment

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

truthfully this is postBuild if build was successful, should we also have a buildFailed or should these actually occur in finally

Copy link
Member

Choose a reason for hiding this comment

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

I think postBuild should always be happy path (non failure) because the addons expect the directory to be provided, but definitely agree that we should add a buildFailed hook in a catch.

I also, think that this PR adds the proper foundation, and we can add buildFailed in a subsequent PR.

@stefanpenner
Copy link
Contributor

awesome thank you.

We can add error instrumentation later (if we need).

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.

3 participants