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

Send complete event #29

Merged
merged 5 commits into from
Feb 19, 2021
Merged

Conversation

thspinto
Copy link
Contributor

@thspinto thspinto commented Feb 18, 2021

β˜• Purpose

Send complete github event in client_payload.data at createEventDispatch.

πŸ” Use cases

I'd like to run sonarscanner to check for code health globally in the organization. For this I need metadata such as the PR number. I thought it would be best to send everything since I sure I'll have a use case for other infos in the future.

@SvanBoxel
Copy link
Owner

I think this would be a really great improvement, so thank you! πŸ’―

What I'm however a bit worried about is backwards compatibility. There are users that leverage the client_payload.sha and client_payload.repository.owner data and changing this will break at least a handful of workflows. Could we instead add a payload or data property to the current payload that includes all event data?

@thspinto
Copy link
Contributor Author

thspinto commented Feb 18, 2021

I liked the idea!

Updated the PR using the client_payload.data field. Also added this information to the README.

@thspinto thspinto marked this pull request as ready for review February 18, 2021 20:49
Copy link
Owner

@SvanBoxel SvanBoxel left a comment

Choose a reason for hiding this comment

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

Love it and we're almost there. I requested a couple of minor changes and then we're good to go. πŸš€

...data,
token: token.data.token
data: context.payload
Copy link
Owner

Choose a reason for hiding this comment

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

I think naming is a bit confusing because we use the spread operator on data on line 67. I wouldn't use a variable and a object property within the same context if they have a different value. Could we either rename the object property or otherwise the data variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it to event. I thought about origin_eventor original_event, but I think that in the context of a repository dispatch just event shouldn't be misleading.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
thspinto and others added 3 commits February 19, 2021 09:19
Copy link
Owner

@SvanBoxel SvanBoxel left a comment

Choose a reason for hiding this comment

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

🚒

@SvanBoxel SvanBoxel merged commit e748722 into SvanBoxel:main Feb 19, 2021
@thspinto thspinto deleted the send-complete-event branch February 19, 2021 15:00
@SvanBoxel
Copy link
Owner

Release with 1.3.0.

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.

2 participants