Skip to content

[Workflow] Adding workflow name to the announce event #23593

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Workflow] Adding workflow name to the announce event #23593

wants to merge 2 commits into from

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jul 19, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Im not sure why this as not been added before. When dispatching all other events we use the forth parameter to Event.

Ping @lyrixx

@lyrixx
Copy link
Member

lyrixx commented Jul 19, 2017 •
edited
Loading

good catch.

Im not sure why this as not been added before. When dispatching all other events we use the forth parameter to Event.

Because I only searched for $event = new Event($subject, $marking, $transition); ($transition vs $initialTransition) ;)

@GromNaN
Copy link
Member

GromNaN commented Jul 19, 2017 •
edited
Loading

A unit test sounds necessary here.

@nicolas-grekas
Copy link
Member

For which branch is this?

@Nyholm
Copy link
Member Author

Nyholm commented Jul 20, 2017 •
edited
Loading

This feature was introduced in #21925

I was not sure if my PR is a bugfix or a feature. We may consider it as a bugfix since all other events have the workflow name. We could also consider it as a feature since announce event never had the workflow name.

If we think this is a bugfix, then branch 3.3. Sorry for not being clear it the PR description.

@lyrixx
Copy link
Member

lyrixx commented Jul 20, 2017

👍
For me it's 3.4, as it's a new feature. And no-one reported it so...

@lyrixx lyrixx changed the title Adding workflow name to the announce event [Workflow] Adding workflow name to the announce event Jul 20, 2017
@nicolas-grekas
Copy link
Member

a new feature

Then a CHANGELOG entry would be good to advertise it :)

@xabbuh xabbuh added this to the 3.4 milestone Jul 21, 2017
@xabbuh xabbuh added Feature and removed Bug labels Jul 21, 2017
@Nyholm
Copy link
Member Author

Nyholm commented Jul 24, 2017

Sorry for the delay. I've updated the change log.

@lyrixx
Copy link
Member

lyrixx commented Jul 25, 2017

👍

javiereguiluz
javiereguiluz approved these changes Jul 26, 2017
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

But we probably need a test before merging. Thanks!

@lyrixx
Copy link
Member

lyrixx commented Jul 26, 2017

Other events are not tested. I'm fine with this PR.
But if @Nyholm wants to add, I will not say no ;)

@Nyholm
Copy link
Member Author

Nyholm commented Jul 26, 2017

I've added a small test.

@lyrixx lyrixx changed the base branch from master to 3.4 July 26, 2017 12:28
@lyrixx
Copy link
Member

lyrixx commented Jul 26, 2017

Oups, I have changed the "base" of this PR to set 3.4 instead of master.
But it looks like github does not like it. Could you please move all your commit on the 3.4 branch and push -f ? If you can't I will do it for you

@Nyholm
Copy link
Member Author

Nyholm commented Jul 26, 2017

Im traveling atm, I maybe able to do it later tonight. Feel free to do it for me.

@lyrixx
Copy link
Member

lyrixx commented Jul 26, 2017

The PR is ready. I'm gonna merge it today.

Thanks.

@xabbuh
Copy link
Member

xabbuh commented Jul 26, 2017

deps=low tests are failing

@Nyholm
Copy link
Member Author

Nyholm commented Jul 27, 2017

Thank you. I've updated the tests now.

@lyrixx
Copy link
Member

lyrixx commented Jul 27, 2017

Thanks for your work on this new feature!

lyrixx added a commit that referenced this pull request Jul 27, 2017
@lyrixx
feature #23593 [Workflow] Adding workflow name to the announce event …
8110598 
…(Nyholm)

This PR was squashed before being merged into the 3.4 branch (closes #23593).

Discussion
----------

[Workflow] Adding workflow name to the announce event

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Im not sure why this as not been added before. When dispatching all other events we use the forth parameter to Event.

Ping @lyrixx

Commits
-------

f4c5cff [Workflow] Adding workflow name to the announce event
@lyrixx lyrixx closed this Jul 27, 2017
@Nyholm
Copy link
Member Author

Nyholm commented Jul 27, 2017

Thank you for merging

@Nyholm Nyholm deleted the patch-3 branch July 27, 2017 08:32
This was referenced Oct 18, 2017
Merged
Merged
@Guite Guite mentioned this pull request Dec 13, 2017
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Feature Status: Needs Review Workflow
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
7 participants
@Nyholm @lyrixx @GromNaN @nicolas-grekas @xabbuh @javiereguiluz @carsonbot