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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/closed issues remove triage label #14

Open
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@nWidart
Copy link

commented May 16, 2019

Hello,

This project seems very useful, after seeing issue #7 I thought I'd give it a shot. 馃槃

This adds the logic to remove the waiting for triage badge if an issue is closed.

nWidart added some commits May 15, 2019

@wilkinsona

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Thanks very much for the pull request. Unfortunately, I think asking GitHub for all closed issues will cause the Bot to exceed GitHub's rate limit. To avoid that, rather than asking for all issues, I think it will be necessary to make a request for closed issues with the waiting for triage label. I'm not yet quite sure how to do that as it's at a level of detail that the repository monitor shouldn't really know about.

@nWidart

This comment has been minimized.

Copy link
Author

commented May 16, 2019

Ah I see, seems like we could call /issues?state=closed&labels=status: waiting-for-triage (would have to encode that)

I had added another getClosedIssues method on the GithubOperations interface at first, but ended up returning all instead from getIssues, and having a condition on the issue.state.

We could have a getClosedIssuesWithLabel for example, combine the results from all open issues and "all closed issues with label x" together, which would allow keeping the same logic in the RepositoryMonitor class but only change the "data fetching" part.

@nWidart

This comment has been minimized.

Copy link
Author

commented May 16, 2019

Actually, that can't work because of the pagination, so I'm thinking it might need to be something like this:

	@Scheduled(fixedRate = 5 * 60 * 1000)
	void monitor() {
		for (Repository repository : this.repositories) {
			monitorOpenIssues(repository);
			monitorClosedIssues(repository);
		}
	}

nWidart added some commits May 16, 2019

Using the query filter to only get closed issues with a specific label
This is in order to reduce the amount of data retrieved from Github and avoid rate limiting
@wilkinsona

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

Thanks again for the PR. This is definitely going in the right direction, but, as I said above I'm not keen on the RepositoryMonitor knowing about the waiting for triage label as it's a level of detail that it should not know about.

I think we should probably decouple things a bit by having the RepositoryMonitor call something (RepositoryListener?) for each Repository, these listeners can then do whatever they want. One general implementation would get all of the open issues and call each IssueListener. Another triage-specific implementation would get all the closed issues with the triage label and remove it.

What do you think? If you are in agreement, would you like to update your PR to this effect?

@nWidart

This comment has been minimized.

Copy link
Author

commented May 23, 2019

That sounds interesting yeah. This would indeed move the responsibility of knowing what an issue is out of the RepositoryMonitor into the listeners.

So the RepositoryMonitor would only loop over the list of RepositoryListener if I understand you correctly.
I'll look into integrating these changes.

Thanks a lot for taking the time to provide feedback! 馃憤

nWidart added some commits May 23, 2019

Extracting common issue monitoring logic into its own class
Inlining the functional interface as it wont be used anywhere else.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.