Deal with github bridge import rate limit - #585

I might be wrong but when you have a loop pushing to a channel like that, you might need to wrap it a select that also monitor the context, so that the loop can exit properly when needed.

Something like that:

	for _, foo := range bar {
		// do something with foo
		foo2 := foo + "hello"
		
		select {
		case <-ctx.Done():
			return
		case out<-foo2:
		}
	}

This might explain why the import doesn't stop properly like in #585 (comment)

Yes, you are right, I included it in one of my commits.

So you moved the credential loading + client creation in ImportAll. Not what I imagined but I guess that make sense.
My tiny annoyance with that though is that this has now a different behavior than the other bridges. Could you make the same change there as well?

My original intention was to simplify the code. Looking at it now makes me also wonder if it really makes any significant difference. The difference in the local code seems to be: When doing the credential loading + client creation in ImportAll, then we do not have to save a *githubv4.Client variable in the githubImporter struct (importMediator already holds the *githubv4.Client), and there is no risk in having two non-identical *cache.RepoCache parameters in the calls to Init and ImportAll.

I could undo this change and move the credential loading + client creation back into Init if that's what you prefer. Please let me know again what you prefer: (1) move credential loading + client creation back into Init, or (2) keeping it in ImportAll and make the same change in all the other bridges.

As you said there is not much difference so both is fine to me as long as it's consistent. (2) has the nice property of avoiding the extra value in the struct, so let's do that one?

I did that with 3d14e2e for the Gitlab and Jira bridges. There was no need to change anything in the Launchpad bridge.

nitpicking: it'd be great if you could explain why this structure with look ahead is necesary

Before I add the comment to the code I would like to ask you for additional input: The reason for using a look ahead element is that in case a github issue is followed by a github issue edit, then in the call to NewBugRaw we use both, the github issue and the first github issue edit, to create the bug instance. This behaviour originates from the previous versions of the github bridge and I adopted it. It seems that this is due to the way github serves its issues on the API. (The same is true for comments.) My question is the following: Is this behaviour necessary? Would it be okay to simply create a (possibly empty) bug instance and then apply the first github issue edit in order to create the issue/bug content. This would cause the edit-history of a github issue and a corresponding git-bug bug instance to not be equal (one might display without "edit" and the other might display with "edit"). I assume that would not be acceptable - right?

Isn't that because on github it's sort of as if the initial creation of an issue is (creation+comment), while in git-bug it's the same operation? I don't remember exactly.

I assume that would not be acceptable - right?

At least not desirable. But your code does that properly at the moment, no?

Just a thought, but there might be a way to simplify. Instead of maintaining two two values currEvent and nextEvent:

• simply iterate on the event stream
• when you know you need an extra event, consume an extra one

Isn't that because on github it's sort of as if the initial creation of an issue is (creation+comment), while in git-bug it's the same operation? I don't remember exactly.

In github the issue holds the most recent content. Because we want to imitate the history, we sometimes create the bug instance from the plain github issue (if there are no edits in github) and sometimes we create the bug from the github issue together with its first edit (if there are edits in github).
But that's also the reason why we do not know whether or not we need the next event before we looked at it.

I assume that would not be acceptable - right?

At least not desirable. But your code does that properly at the moment, no?

Yes, it is handled properly.

If you want to get rid of the look ahead, then I think it would be best to make importMediator provide more context and add a field (say hasEdits bool) to IssueEvent. Then we just know whether or not there are edits and we do not have to look ahead. (And we would have to add a similar data field to TimelineEvent, because it holds comments which might be followed by comment edits. It's the same situation.)

For now I only added a comment to the code with ede5e21 explaining why look ahead is necessary. Let me know what you think.

The bridge itself should not write anything, this should be left to do by the various UI (CLI, termui, webui). Instead, there is this out channel where you write some core.ImportResult (might not be the best naming).
It looks like you have a need for an arbitrary-ish user feedback for the rate limiting? In that case we should define either 1) an event for an arbitrary message ImportEventMessage or 2) an event for the rate limiting ImportEventRateLimiting. The second one makes probably more sense as it would allow the UIs to special case the handling.

I did that with 0fd5701

no big deal, but FYI there is a simpler way: https://stackoverflow.com/a/62287775/6594169

I looked at the docs which made me reflect on the following: With time.NewTimer it is possible to cancel/stop the timer in case it is not needed anymore (e.g., in case the context.Done() channel is closed). That's not possible with the time.After call. The effective difference is that the memory of the timer will be freed only after the timer is really expired. Here that is not a big difference because the goroutine is sleeping only a few seconds. But a bit further down when we are handling the rate limiting, then the goroutine may be sleeping for up to an hour and canceling/stoping the timer might be favorable:
https://github.com/MichaelMure/git-bug/blob/b2e98ef07f3083db303099fbcedbd98a33ad1164/bridge/github/import_mediator.go#L413

This is GREAT. Nice work to have come up with this structure.