Skip to content

GH-3151: Add events to ICakeEngine to invoke at the end of Setup/Teardown#3152

Merged
augustoproiete merged 1 commit intocake-build:developfrom
BlythMeister:EngineEvents
Nov 7, 2021
Merged

GH-3151: Add events to ICakeEngine to invoke at the end of Setup/Teardown#3152
augustoproiete merged 1 commit intocake-build:developfrom
BlythMeister:EngineEvents

Conversation

@BlythMeister
Copy link
Copy Markdown
Contributor

@BlythMeister BlythMeister commented Feb 16, 2021

This allows for any logic required after each section to be executed.

Fixes #3151

@augustoproiete augustoproiete changed the title Add events to ICakeEngine to invoke at the end of Setup/Teardown GH-3151: Add events to ICakeEngine to invoke at the end of Setup/Teardown Feb 28, 2021
@augustoproiete
Copy link
Copy Markdown
Member

augustoproiete commented Feb 28, 2021 •
edited
Loading

@BlythMeister Thank you for the PR!

Just to give you some feedback on this one, we discussed this PR internally and given that it introduces a breaking change to the public API (i.e. ICakeEngine) which would impact all extensions, we'll consider it for Cake v2.0.0 later this year, at which time we'll engage with you to understand more about the use-case of these events and test some examples together.

@augustoproiete augustoproiete marked this pull request as draft February 28, 2021 01:30
@pascalberger pascalberger added this to the Next Major Candidate milestone Oct 11, 2021
@pascalberger pascalberger removed this from the Next Major Candidate milestone Oct 11, 2021
@pascalberger
Copy link
Copy Markdown
Member

pascalberger commented Oct 17, 2021

Added a comment to the issues where to discuss the requirements and design first: #3151 (comment)

@pascalberger pascalberger requested review from augustoproiete and nils-a and removed request for augustoproiete October 24, 2021 19:25
@augustoproiete augustoproiete self-requested a review October 24, 2021 19:26
nils-a
nils-a previously requested changes Oct 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@nils-a nils-a left a comment

Choose a reason for hiding this comment

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

@BlythMeister thank you for the contribution 👍

I added some requests for changes.

@augustoproiete augustoproiete removed their request for review October 26, 2021 00:12
@nils-a nils-a force-pushed the EngineEvents branch 2 times, most recently from 4cfb547 to 4a7b627 Compare November 5, 2021 14:13
@nils-a
Copy link
Copy Markdown
Member

nils-a commented Nov 5, 2021

@cake-build/cake-team I implemented the discussed modifications to have this PR ready for the next release. Can I get another pair of eyes on this?

@BlythMeister I hope it's ok for you that I'd gone forth and implemented the changes myself.

@nils-a nils-a marked this pull request as ready for review November 5, 2021 14:20
@BlythMeister
Copy link
Copy Markdown
Contributor Author

BlythMeister commented Nov 5, 2021

Yeah perfectly fine with me. It's been on my to-do list...but work has been super busy!

@nils-a nils-a dismissed their stale review November 5, 2021 14:21

Done the changes, shouldn't review them :)

gep13
gep13 approved these changes Nov 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

augustoproiete
augustoproiete requested changes Nov 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

Looks good. Added a couple of comments

@BlythMeister
Copy link
Copy Markdown
Contributor Author

BlythMeister commented Nov 6, 2021

Who is to resolve the comments?
I'm now not sure what I did originally and what was modified.

If you want to wait for me, I'm not sure what timeline that will be, as I am super busy at work at the moment.

@pascalberger
Copy link
Copy Markdown
Member

pascalberger commented Nov 7, 2021

Who is to resolve the comments? I'm now not sure what I did originally and what was modified.

If you want to wait for me, I'm not sure what timeline that will be, as I am super busy at work at the moment.

@BlythMeister That's fine. I took care of resolving the comments

@BlythMeister @devlead
Add events to ICakeEngine to invoke at the end of Setup/Teardown
f009de9 
The original Setup, Teardown, TaskSetup and TaskTeardown were marked
obsolete and new BeforeSetup, BeforeTeardown, BeforeTaskSetup
BeforeTaskTeardown were introduced to complement the new
AfterSetup, AfterTeardown, AfterTaskSetup and AfterTaskTeardown
augustoproiete
augustoproiete approved these changes Nov 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

LGTM!

@augustoproiete augustoproiete merged commit 3b472df into cake-build:develop Nov 7, 2021
@augustoproiete
Copy link
Copy Markdown
Member

augustoproiete commented Nov 7, 2021

@BlythMeister your changes have been merged, thanks for your contribution 👍

@nils-a nils-a mentioned this pull request Nov 15, 2021
Closed
@BlythMeister BlythMeister deleted the EngineEvents branch January 20, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pascalberger pascalberger pascalberger left review comments

@augustoproiete augustoproiete augustoproiete approved these changes

@gep13 gep13 gep13 approved these changes

@nils-a nils-a nils-a left review comments

Assignees

@nils-a nils-a

Labels

Breaking change v2.x Candidate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for Engine event hooks after execution as well as before

5 participants

@BlythMeister @augustoproiete @pascalberger @nils-a @gep13