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 event processing to cli get issues #692

Closed
wants to merge 1 commit into from
Closed

Add event processing to cli get issues #692

wants to merge 1 commit into from

Conversation

elkuku
Copy link
Contributor

@elkuku elkuku commented Aug 10, 2015

This will add event handling for the CLI process that get issues (and PRs).

The comment event is not implemented yet since its functionality is not clear to me. From what I can see it tries to add a missing behavior like an event "onAfterIssueSave" or something that triggers changes made by jissues?

I hope this brings a bit closer to unify the handling in hooks and cron jobs.

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2015

The comment event is not implemented yet since its functionality is not clear to me.

On the comment event we listen on every comment and do some checks. So the comment event is just the trigger to run some checks (e.g. auto RTC label)

From what I can see it tries to add a missing behavior like an event "onAfterIssueSave" or something that triggers changes made by jissues?

Yes. But this event needs also run if we have a change (new comment) via Github ;)

@elkuku
Copy link
Contributor Author

elkuku commented Aug 10, 2015

Well what is not clear to me is the way the JoomlacmsCommentsListener is supposed to work. Currently it checks for a change made on jissues (the status changed to RTC) when a comment is added (or modified perhaps) on GitHub not doing anything with the comment but with the issue (which is expect as a parameter)

In other words:
If you change the status to RTC on jissues you have to add a comment so the RTC label will appear on GitHub?
This feels wrong to me :(

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2015

If you change the status to RTC on jissues you have to add a comment so the RTC label will appear on GitHub?

Yes this is the current way. If you have a better way 👍

But it is nothing critical. If I or others add the RTC label they add a comment like RTC based on tests or anything like this ;)

@mbabker
Copy link
Contributor

mbabker commented Aug 10, 2015

Separate PR but the RTC label shouldn't be checked on the comment event
(things like category, priority, and status should be editable without
comments). But, that one gets into a funny scenario where we check
something not on GitHub to set something on GitHub so it's definitely a
little awkward. As we get events better sorted this might be something to
move to our save handlers and not even be dependent on a webhook to trigger.

On Monday, August 10, 2015, zero-24 [email protected] wrote:

If you change the status to RTC on jissues you have to add a comment so
the RTC label will appear on GitHub?

Yes this is the current way. If you have a better way [image: 👍]

But it is nothing critical. If I or others add the RTC label they add a
comment like RTC based on tests or anything like this ;)


Reply to this email directly or view it on GitHub
#692 (comment).

@elkuku
Copy link
Contributor Author

elkuku commented Aug 10, 2015

Yes I guess that is what I was tying to say 😉

Regarding this PR: Do you think that it would make any sense trying to implement a trigger for the comment event listeners in the CLI script? or any triggers at all??

I also have concerns about the additional requests being made since currently the "label check" results are not cached and will result in a lot of additional requests when batch processing issues in a cron job...

@elkuku elkuku mentioned this pull request Aug 15, 2015
@elkuku elkuku closed this Mar 29, 2017
@elkuku elkuku deleted the cli-events branch June 24, 2018 21:51
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