Bug 395059 - support Gerrit 2.6 / Eclipse.org Reviews
Summary: support Gerrit 2.6 / Eclipse.org Reviews
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P2 enhancement with 8 votes (vote)
Target Milestone: 2.0.1   Edit
Assignee: Tomasz Zarna CLA
QA Contact:
URL: http://wiki.eclipse.org/Mylyn/Reviews...
Whiteboard: sprint=6;
Keywords: plan
Depends on: 414078 414647 415079 415278
Blocks:
  Show dependency tree
 
Reported: 2012-11-26 07:05 EST by Sascha Scholz CLA
Modified: 2013-08-19 12:13 EDT (History)
28 users (show)

See Also:


Attachments
AllGerritTests against http://mylyn.org/gerrit-2.6-rc3/ (97.91 KB, application/xml)
2013-06-20 11:47 EDT, Tomasz Zarna CLA
no flags Details
AllGerritTests against http://mylyn.org/gerrit-2.7-rc1/ (90.76 KB, application/xml)
2013-06-20 11:47 EDT, Tomasz Zarna CLA
no flags Details
mylyn/context/zip (192.24 KB, application/octet-stream)
2013-06-21 13:52 EDT, Tomasz Zarna CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sascha Scholz CLA 2012-11-26 07:05:45 EST
Make the Gerrit connector compatible with Gerrit 2.6. Known incompatibilities:

* Internal RPC interface has changed from /gerrit/rpc to /gerrit_ui/rpc [1,2]

[1] https://gerrit-review.googlesource.com/38400
[2] https://gerrit-review.googlesource.com/38562
Comment 1 Steffen Pingel CLA 2012-11-26 07:34:04 EST
We haven't even closed the 2.5 bug ;). Sounds easy enough though if that's the only change so far.
Comment 2 Shawn Pearce CLA 2012-11-26 13:32:37 EST
(In reply to comment #1)
> We haven't even closed the 2.5 bug ;). Sounds easy enough though if that's
> the only change so far.

There are more changes in 2.6:

ProjectAdminService.addBranch changed its result type.

ChangeManageService.abandonChange no longer exists.
ChangeManageService.restoreChange no longer exists.
ChangeManageService.revertChange no longer exists.
- These three replaced by JSON POST to /changes/{id}/{abandon,restore,revert}

PatchDetailService.publishComments no longer exists.
- Replaced by JSON POST to /changes/{id}/revisions/{sha1}/review

The meaning of the "id" field changed in the JSON results from /changes/. It now means the triplet "project~branch~change_id". A new change_id field was added to hold the raw Change-Id string ("Ic0ffee....").

You can tell a 2.6 response vs. a 2.5 response by looking at the output from /changes/, if at least one result is returned a 2.6 server returns a "kind" and a "change_id" field in the JSON objects. A 2.5 server doesn't have these.

I expect even more changes in 2.6 as we rewrite the protocol the web UI client uses to talk to the server to decouple it from the ugly JSON-RPC we use and switch to a simpler REST JSON API.
Comment 3 Steffen Pingel CLA 2013-04-18 08:29:05 EDT
I have provisioned a Gerrit 2.6-rc0 test repository at http://mylyn.org/gerrit-2.6-rc0/#/. Unfortunately we can't currently test against it due to this regression in Gerrit 2.6 which causes the login page to loop endlessly with redirects: https://code.google.com/p/gerrit/issues/detail?id=1862 .

I'll take this off the plan for now since we have limited time left until the 3.9 release and currently no one to work on this.
Comment 4 Christoph Pohl CLA 2013-04-22 02:39:08 EDT
Hi folks,
it's a bit sad to hear that this will be postponed further. Essentially this means that users forced to work with new Gerrit releases won't be able to use the Mylyn Connector at all...
Cheers,
Christoph
Comment 5 Miles Parker CLA 2013-04-22 12:12:05 EDT
We'd all love to see this support in. The good news is that with the new Remote API the server interaction is well isolated and it is all under testing. It should also be quite easy for someone to work with the code who isn't familiar w/ the rest of the Mylyn API. So definitely a place where someone could make a nice contribution... :)
Comment 6 Steffen Pingel CLA 2013-05-08 14:31:53 EDT
We now have test instances running here:

http://mylyn.org/gerrit-2.6-rc2/#/
http://mylyn.org/gerrit-2.7-rc0/#/
Comment 7 Miles Parker CLA 2013-06-14 13:25:15 EDT
Shawn, can you give us an update on where 2.6 ended up and any further changes? 

In order to move forward on this, I think we need to think about where we are with the REST API on 2.6. The last message from Shawn makes it seem that 2.6 is sort of part JSON-RPC and part REST? The REST API info I've found looks like it's aimed at 2.7 consumers. I think what I'm asking is, would it make more sense to go all REST now, or can we go w/ JSON and have a clean break to REST in 2.7..
Comment 8 Edwin Kempin CLA 2013-06-17 03:02:28 EDT
Miles, I can confirm that in Gerrit 2.6 the REST API does not yet cover all functionality. There are still a lot of old JSON-RPC's. At best have a look at the 2.6 REST API documentation [1] to get an impression about what is available with 2.6.
With 2.7 only a few new REST endpoints were added [2], but I think they may be relevant for the Gerrit Mylyn Connector. The complete 2.7 REST API documentation is available at [3].
Gerrit 2.8 will support some more REST endpoints [4]. Most important for you is probably the ability to get diffs from Gerrit [5].
It's been already some time since I looked at the code of the Mylyn Gerrit Connector, but I believe a few RPCs that you use are still not available over REST in current master, namely IIRC getting contributor agreements and getting the server configuration. But all the rest should be covered.

[1] http://gerrit-documentation.googlecode.com/svn/Documentation/2.6/rest-api.html
[2] http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.7.html#_rest_api
[3] http://gerrit-documentation.googlecode.com/svn/Documentation/2.7/rest-api.html
[4] http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.8.html#_rest_api
[5] https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-diff
Comment 9 Miles Parker CLA 2013-06-17 12:19:54 EDT
Thanks, that's really really helpful.

(In reply to comment #8)
> Miles, I can confirm that in Gerrit 2.6 the REST API does not yet cover all
> functionality. There are still a lot of old JSON-RPC's. At best have a look at
> the 2.6 REST API documentation [1] to get an impression about what is available
> with 2.6.

Conversely, do you think it's a possible to use all JSON-RPC at this point for pull (GET)? It looks like a lot of the new REST stuff is on the POST side which for obscure reasons should be a little more amenable to a mix and match approach, but for the pull side it really will be much simpler to choose one or the other.

> With 2.7 only a few new REST endpoints were added [2], but I think they may be
> relevant for the Gerrit Mylyn Connector. The complete 2.7 REST API documentation
> is available at [3].
> Gerrit 2.8 will support some more REST endpoints [4]. Most important for you is
> probably the ability to get diffs from Gerrit [5].

+1!

> It's been already some time since I looked at the code of the Mylyn Gerrit
> Connector, but I believe a few RPCs that you use are still not available over
> REST in current master, namely IIRC getting contributor agreements and getting
> the server configuration. But all the rest should be covered.

If there is some additional API for configuration stuff that really isn't as big of an issue. The most important thing is that all of the stuff for working with the core Reviews and Patch Sets are available, as that is the part of the code that is most abstracted on our side (through our "Remote API").

> [1]
> http://gerrit-documentation.googlecode.com/svn/Documentation/2.6/rest-api.html
> [2]
> http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.7.html#_rest_api
> [3]
> http://gerrit-documentation.googlecode.com/svn/Documentation/2.7/rest-api.html
> [4]
> http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.8.html#_rest_api
> [5]
> https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-diff
Comment 10 Miles Parker CLA 2013-06-17 12:25:18 EDT
As a note for Mylyn Reviews team, we should determine the level of backward compatibility we'll want to preserve. One of the trickier aspects of all of this will be to make the different combinations of REST and JSON work in a manageable way against the Remote API, with what might be a lot of special cases. I hesitate to put in yet another abstraction layer (such as an adapter/decorator for the AbstractRemoteEmfFactory) to handle the various possibilities, but we may need to consider using some kind of wrapper class that can contain either/both the JSON object and some object for the REST response.
Comment 11 Edwin Kempin CLA 2013-06-18 02:28:05 EDT
> Conversely, do you think it's a possible to use all JSON-RPC at this point
> for pull (GET)?
Listing of changes can't be done with JSON-RPC anymore [1], but this was already in 2.5 the case and I think your GerritClient class already knows how to do it via REST. Anyway you will still need a minor modification of this [2].
Other than that the JSON-PRC for getting groups and group details was removed [3,4]. Not sure if you use this.
Else it might be possible to stay in 2.6 with pulling data through JSON-RPC, but I'm not completely sure.

[1] https://gerrit-review.googlesource.com/34534
[2] https://gerrit-review.googlesource.com/43202
[3] https://gerrit-review.googlesource.com/40854
[4] https://gerrit-review.googlesource.com/41940
Comment 12 Miles Parker CLA 2013-06-18 12:18:20 EDT
Thanks again. Naturally we'd like to make as clean a break as possible but it sounds like we just need to start experimenting to see what we can do. And yep we already have support for REST based query.
Comment 13 Tomasz Zarna CLA 2013-06-18 12:35:24 EDT
Lemme try tackle this one.
Comment 14 Miles Parker CLA 2013-06-18 12:48:44 EDT
(In reply to comment #13)
> Lemme try tackle this one.

Yay, Tomek! :)
Comment 15 Tomasz Zarna CLA 2013-06-20 11:47:12 EDT
Created attachment 232603 [details]
AllGerritTests against http://mylyn.org/gerrit-2.6-rc3/
Comment 16 Tomasz Zarna CLA 2013-06-20 11:47:47 EDT
Created attachment 232604 [details]
AllGerritTests against http://mylyn.org/gerrit-2.7-rc1/
Comment 17 Tomasz Zarna CLA 2013-06-20 11:54:30 EDT
Started with running AllGerritTests against Gerrit 2.6 and 2.7 to see how serious the situation is. Most of the test fails at early stage during setup/init phase. In both cases 18 out 26 tests either failed or errored. This leaves a couple of tests passing but my guess is that they shouldn't be part of a fixture in the first place.
Comment 18 Tomasz Zarna CLA 2013-06-20 12:05:49 EDT
Forgot to mention that I'm not able to validate a defined repository for either Gerrit 2.6 and 2.7 (with and without creds).
Comment 19 Edwin Kempin CLA 2013-06-21 01:04:56 EDT
The validation of the connection to a Gerrit 2.6+ fails due to [1] which did a rename in the URL path of the old internal RPC interface from /gerrit/rpc to /gerrit_ui/rpc.

[1] https://gerrit-review.googlesource.com/38400
Comment 20 Miles Parker CLA 2013-06-21 12:50:06 EDT
The first thing we're going to want is a simple API to determine target API version from the hints provided in e.g. comment 2.
Comment 21 Sam Davis CLA 2013-06-21 12:58:41 EDT
Ideally we would find out the version when validating the connection, otherwise this will create a lot of issues.
Comment 22 Tomasz Zarna CLA 2013-06-21 13:27:12 EDT
A dumb question: isn't the version part of GerritConfiguration, got after calling GerritClient.refreshConfig(IProgressMonitor)?

btw: a quick and dirty patch for making the connector (partially) work with Gerrit 2.6: https://git.eclipse.org/r/#/c/13982
Comment 23 Tomasz Zarna CLA 2013-06-21 13:52:55 EDT
Created attachment 232662 [details]
mylyn/context/zip
Comment 24 Miles Parker CLA 2013-06-24 15:33:23 EDT
As anticipated, as of the closure of bug 408640 five minutes ago, it we no longer work with Eclipse Reviews. :( 

Needless to say, we're aware that this one is top priority for community and we still anticipate having full report for our 2.0.1 maintenance release. Note that this will lead to some error messages that might be misleading (see bug 411422) but are likely to be because of basic connectivity issues. We've also already closed many bugs for the 2.0.1 maintenance release, so *there is no need to file a seperate bug against 2.0.0 for https://git.eclipse.org/r/#/ unless you feel it is unrelated* to the basic API incompatibility issue.

Sorry for the inconvenience, everyone!
Comment 25 Sam Davis CLA 2013-06-26 21:12:32 EDT
I've noticed that eclipse.org's Gerrit now has support for "file comments" (yellow button in top left and right corner when comparing). We may want to add support for these in the connector too.
Comment 26 Miles Parker CLA 2013-06-27 11:35:45 EDT
(In reply to comment #25)
> I've noticed that eclipse.org's Gerrit now has support for "file comments"
> (yellow button in top left and right corner when comparing). We may want to
> add support for these in the connector too.

Good idea. Open a separate bug for this?
Comment 27 Tomasz Zarna CLA 2013-07-01 19:02:11 EDT
(In reply to comment #26)
> Good idea. Open a separate bug for this?

It's bug 412058.
Comment 28 Tomasz Zarna CLA 2013-07-02 11:35:20 EDT
I replaced the quick and dirty hacks in https://git.eclipse.org/r/#/c/13982 with proper reviews:
* https://git.eclipse.org/r/14181 -- remove deprecated format=JSON query parameter
* https://git.eclipse.org/r/14183 -- prefer REST API when running queries
* https://git.eclipse.org/r/14191 -- fix repository validation (authentication)
Comment 29 Miles Parker CLA 2013-07-02 12:35:12 EDT
(In reply to comment #28)
> I replaced the quick and dirty hacks in https://git.eclipse.org/r/#/c/13982 with
> proper reviews:
> * https://git.eclipse.org/r/14181 -- remove deprecated format=JSON query
> parameter
> * https://git.eclipse.org/r/14183 -- prefer REST API when running queries
> * https://git.eclipse.org/r/14191 -- fix repository validation (authentication)


Cool! I'll try to take a look later today. (I think Sam is out for the week.)
Comment 30 Tomasz Zarna CLA 2013-07-02 12:39:32 EDT
(In reply to comment #29)
> I think Sam is out for the week.

That is correct.
Comment 31 Miles Parker CLA 2013-07-02 13:43:20 EDT
(In reply to comment #28)
> I replaced the quick and dirty hacks in https://git.eclipse.org/r/#/c/13982 with
> proper reviews:
> * https://git.eclipse.org/r/14181 -- remove deprecated format=JSON query
> parameter
> * https://git.eclipse.org/r/14183 -- prefer REST API when running queries
> * https://git.eclipse.org/r/14191 -- fix repository validation (authentication)

Tomek, please see my comments. Mostly, I'm suggesting that we provide an abstraction for the concern of API version. We'll need that in order to support selection of different Remote API strategies. And I need to give some thought to how to support switching those APIs as appropriate. Hopefully that won't take too much re-engineering of the Remote API stuff.
Comment 32 Tomasz Zarna CLA 2013-07-03 07:03:38 EDT
(In reply to comment #31)
> And I need to give some thought to
> how to support switching those APIs as appropriate.

Feel free to share it on http://wiki.eclipse.org/Mylyn/Reviews/Gerrit_2.6, a new wiki page where I jotted down all the hiccups found while trying to get the connector running with Gerrit 2.6
Comment 33 Miles Parker CLA 2013-07-03 19:05:33 EDT
Tomek, sometimes it is better to show rather than tell :), so I've done the refactoring I mentioned in other reviews so that you can get a better idea of what I'm talking about.

https://git.eclipse.org/r/#/c/14241/
Comment 34 Steffen Pingel CLA 2013-07-04 05:04:17 EDT
(In reply to comment #33)
> Tomek, sometimes it is better to show rather than tell :), so I've done the
> refactoring I mentioned in other reviews so that you can get a better idea of
> what I'm talking about.
> 
> https://git.eclipse.org/r/#/c/14241/

I don't see the benefit of this refactoring and it's not the kind of the change that should go into a service release. I also don't see it as a pre-requisite for Gerrit 2.6 support. Miles, I would suggest to open another bug and describe how the change you propose improves the architecture and we can consider that for 2.1 once we have reached a consensus.
Comment 35 Tomasz Zarna CLA 2013-07-04 09:00:41 EDT
(In reply to comment #21)
> Ideally we would find out the version when validating the connection

This is covered in https://git.eclipse.org/r/#/c/14232/
Comment 36 Miles Parker CLA 2013-07-04 13:03:31 EDT
(In reply to comment #34)
> (In reply to comment #33)
> > Tomek, sometimes it is better to show rather than tell :), so I've done the
> > refactoring I mentioned in other reviews so that you can get a better idea of
> > what I'm talking about.
> >
> > https://git.eclipse.org/r/#/c/14241/
> 
> I don't see the benefit of this refactoring and it's not the kind of the change
> that should go into a service release. I also don't see it as a pre-requisite
> for Gerrit 2.6 support. Miles, I would suggest to open another bug and describe
> how the change you propose improves the architecture and we can consider that
> for 2.1 once we have reached a consensus.

It's intended as an alternative proposal to Tomek's https://git.eclipse.org/r/#/c/14229/. It does the same thing as that large refactoring, without creating a new abstraction layer. We'll need some way to select behaviour based on versioning, and I'm suggesting that this may be the simplest way to make that work.
Comment 37 Steffen Pingel CLA 2013-07-04 13:36:02 EDT
(In reply to comment #36)
> It's intended as an alternative proposal to Tomek's
> https://git.eclipse.org/r/#/c/14229/. It does the same thing as that large
> refactoring, without creating a new abstraction layer. 

I dislike that proposal for the same reasons.

> We'll need some way to
> select behaviour based on versioning, and I'm suggesting that this may be the
> simplest way to make that work.

I would suggest to do that in GerritClient for now since the majority of API calls will still go through the legacy JSON-RPC interface and it's very simple to do a version check and delegate. This needs to be cleaned up at some point but we should minimize the number of refactoring in the service branch.
Comment 38 Miles Parker CLA 2013-07-04 14:55:21 EDT
(In reply to comment #37)
> > We'll need some way to
> > select behaviour based on versioning, and I'm suggesting that this may be the
> > simplest way to make that work.

I can see your point if the goal is to minimize actual lines of code changes for the release. But if the goal is to minimize code complexity changes, I think this one would be preferred. There really aren't any functional changes; we're just moving the functionality to where it really belongs to make it easier to select.

> I would suggest to do that in GerritClient for now since the majority of API
> calls will still go through the legacy JSON-RPC interface and it's very simple
> to do a version check and delegate. This needs to be cleaned up at some point
> but we should minimize the number of refactoring in the service branch.

So, if there are indeed just a few new REST calls replacing JSON-RPC, then I think what you're saying makes sense. Perhaps we should split this out into two phases. 1) A simplest thing that could possibly work for 2.6 and 2) refactoring to support a much more generic approach.
Comment 39 Tomasz Zarna CLA 2013-07-04 16:30:20 EDT
(In reply to comment #38)
> Perhaps we should split this out into two phases.

Well, this is exactly how I'm working on it.

> 1) A simplest thing that could possibly work for 2.6 and 

* https://git.eclipse.org/r/14232
* https://git.eclipse.org/r/14183
* https://git.eclipse.org/r/14191

> 2) refactoring to support a much more generic approach.

* https://git.eclipse.org/r/14229 my proposal
* https://git.eclipse.org/r/14241 Miles' idea

I believe 1) and 2) can be developed in parallel, with more focus on 1) of course. Looking into 2) allows to take me a broader view on the issue and see other places that should be possibly covered in 1). 

To make it clear, maybe we should file a separate bug for 2) ?
Comment 40 Steffen Pingel CLA 2013-07-04 17:04:27 EDT
(In reply to comment #39)
> To make it clear, maybe we should file a separate bug for 2) ?

Great. Let's do that and focus the discussion here on getting 2.6 support working. 

Miles, less code is always good but I'm not so much concerned with the number of lines rather than the amount of potential breakage for integrators. We have a policy of minimizing changes to internals between service releases since we never know who is consuming them (or we rather know that integrators tend to do that due to API limitations): http://wiki.eclipse.org/Mylyn/Integrator_Reference#Using_Internals. I'm not against refactoring at all if they help to improve the architecture but if possible please wait until master is switched to 2.1.
Comment 41 Miles Parker CLA 2013-07-04 17:29:31 EDT
(In reply to comment #40)
> (In reply to comment #39)
> > To make it clear, maybe we should file a separate bug for 2) ?
> 
> Great. Let's do that and focus the discussion here on getting 2.6 support
> working.

+1 -- I think this is not a time to make the perfect the enemy of the "good enough".

> http://wiki.eclipse.org/Mylyn/Integrator_Reference#Using_Internals. I'm not
> against refactoring at all if they help to improve the architecture but if
> possible please wait until master is switched to 2.1.

Agreed this should wait until 2.1. For the record, the only thing this change actually breaks is getting rid of ReviewClient which is never called anymore and one method in ReviewsConnector. Id be *very* surprised if anyone were actually consuming this right now, but I appreciate the principle.
Comment 42 Tomasz Zarna CLA 2013-07-05 07:18:28 EDT
The bugzilla for refactoring GerritClient -- bug 412390
Comment 43 Tomasz Zarna CLA 2013-07-08 07:39:53 EDT
Working on https://git.eclipse.org/r/14183 is quite tricky as long as bug 412102 is open since it's hard to tell which errors are caused by 2.6 and which have been there for a while. Lucky for me, Miles is already working on it.
Comment 44 Tomasz Zarna CLA 2013-07-09 11:48:18 EDT
Quick update: with https://git.eclipse.org/r/#/c/14191/2 I'm able to log in to our test repos (2.6-rc3 and 2.7-rc1), but still not able to do the same for git.eclipse.org (2.6.1).
Comment 45 Tomasz Zarna CLA 2013-07-10 11:54:07 EDT
Recap:
Work in progress:
* detect Gerrit version and inform user in case it is not supported: https://git.eclipse.org/r/14232, ready for review
* allow querying changes when anonymous: https://git.eclipse.org/r/14183, kind of blocked by bug 412102
* authentication in Gerrit 2.6 use xGerritAuth if available and try POST to login: https://git.eclipse.org/r/14191, works for test repos and git.eclipse.org, ready for review, but blocked by https://git.eclipse.org/r/14445 in Mylyn Commons
Work left:
* review https://git.eclipse.org/r/#/c/14328/ and https://git.eclipse.org/r/#/c/14294/
* continue working on repository connection validation
* querying changes works (with https://git.eclipse.org/r/14183 applied you can see all open reviews), but each task gets a warning saying "Problem while retrieving Gerrit review.: Not Found"
* opening a review in editor fails with an NPE in AbstractReviewTaskEditorPage
* once editor is in good shape check if all the actions work as expected e.g. review, submit, fetch etc
Comment 46 Tomasz Zarna CLA 2013-07-10 14:13:40 EDT
(In reply to comment #45)
> * continue working on repository connection validation
> * querying changes works, but each task gets a warning saying "Problem while
> retrieving Gerrit review.: Not Found"

Being fixed here: https://git.eclipse.org/r/#/c/14453/
Comment 47 Miles Parker CLA 2013-07-10 15:10:32 EDT
(In reply to comment #45)
> Recap:
> Work in progress:
> * detect Gerrit version and inform user in case it is not supported:
> https://git.eclipse.org/r/14232, ready for review

I reviewed that one. Any other ones you need me to take a look at in next day?
Comment 48 Tomasz Zarna CLA 2013-07-10 15:30:17 EDT
(In reply to comment #47)
> Any other ones you need me to take a look at in next day?

https://git.eclipse.org/r/14445 (in Mylyn Commons) and https://git.eclipse.org/r/14191 (depending on the former)
Comment 49 Steffen Pingel CLA 2013-07-10 16:23:20 EDT
If there a blockers for full 2.6 support it would already help to get task list queries working and a basic editor (even if it can't display full patch set content). Eventually we should fully support 2.6 but depending on the remaining effort we could stage it and release partial support in 3.9.1 at the end of July and full support in 3.9.2 once the implementation is complete. It's just a thought in case it helps prioritize efforts.
Comment 50 Tomasz Zarna CLA 2013-07-10 16:53:11 EDT
Agreed, initially I wanted to focus on anonymous access, but it turned out flaky even with 2.5.x. My main concern now is https://git.eclipse.org/r/#/c/14453/ (and dependent reviews): auth, repo config, task list and eventually editor (in that order).
Comment 51 Miles Parker CLA 2013-07-10 18:05:04 EDT
(In reply to comment #49)
> If there a blockers for full 2.6 support it would already help to get task list
> queries working and a basic editor (even if it can't display full patch set
> content). 

I agree that we should triage these items. Actually, we might want to look at breaking this task into a few sub-tasks, but it might still not be obvious how to do that.

(In reply to comment #50)
> Agreed, initially I wanted to focus on anonymous access, but it turned out flaky
> even with 2.5.x. My main concern now is https://git.eclipse.org/r/#/c/14453/
> (and dependent reviews): auth, repo config, task list and eventually editor (in
> that order).

I think we should fix anoynmous access first, given that it's completely broken, and the bare minimum we should be able to support for 2.6 is non-authenticated searches for tasks and review items. I think without doing that properly, then the authorization and config stuff will only have to be revistied anyway. Please see https://git.eclipse.org/r/#/c/14294/ which makes some key changes in how configuration is handled. My feeling is that the other changes should be based on that, and I'm not saying that just because it's my review. ;D
Comment 52 Tomasz Zarna CLA 2013-07-11 08:04:04 EDT
Side question: since we don't claim to support Gerrit versions older than 2.4, maybe it's a good time* to update com.google.gerrit.common and other bundles from Google?

* not for the SR, but in the near future
Comment 53 Steffen Pingel CLA 2013-07-11 08:32:22 EDT
(In reply to comment #52)
> Side question: since we don't claim to support Gerrit versions older than 2.4,
> maybe it's a good time* to update com.google.gerrit.common and other bundles
> from Google?
> 
> * not for the SR, but in the near future

I don't think it's worth the effort considering that it needs to go through IP review, Orbit etc. I would prefer to create the necessary data model objects in the Gerrit connector source based on the Rest API output/documentation going forward. From experience it's easier to manage particularly when supporting multiple versions.
Comment 54 Tomasz Zarna CLA 2013-07-11 09:07:17 EDT
With https://git.eclipse.org/r/#/c/14453/2/ I'm able to log in to git.eclipse.org (a 2.6 Gerrit server), query for my changes, open a review in editor and inspect diffs. 

The bad news is that I'm not able to fetch a patch set, getting a mysterious "Cannot Fetch. Try re-synchronizing the review task. If that fails, there may be a problem with your repository connection." (no stacktrace) and since Json configuration object has changed in 2.6 (see wiki for more info) I get a RuntimeException in GerritReviewRemoteFactory#updateApprovalsAndRequirements. 

I'm gonna on fixing the latter first, as suggested by Steffen in comment 53, by updating our model objects.
Comment 55 Steffen Pingel CLA 2013-07-11 11:45:52 EDT
That sounds great! I'll take a review pass through the open changes.
Comment 56 Miles Parker CLA 2013-07-11 12:24:53 EDT
(In reply to comment #54)
> The bad news is that I'm not able to fetch a patch set, getting a mysterious
> "Cannot Fetch. Try re-synchronizing the review task. If that fails, there
> may be a problem with your repository connection." (no stacktrace) and since
> Json configuration object has changed in 2.6 (see wiki for more info) I get

Ok, that is all happening because we haven't merged https://git.eclipse.org/r/#/c/13998/ yet. I jsut went ahead and did that.
Comment 57 Steffen Pingel CLA 2013-07-12 18:22:00 EDT
Merged the following reviews:

https://git.eclipse.org/r/14232
https://git.eclipse.org/r/14183
https://git.eclipse.org/r/14191
https://git.eclipse.org/r/14453
https://git.eclipse.org/r/14502

Thanks to Tomek's amazing efforts this make querying and opening of the review editor work again with 2.6 repositories. There are still some open bugs and limitations such as approvals not showing in the editor (this will require an additional rest call) but the basics work. More technical details are on the wiki page: http://wiki.eclipse.org/Mylyn/Reviews/Gerrit_2.6.

A build with the latest changes is available from the p2 repository at http://download.eclipse.org/mylyn/snapshots/weekly/ .
Comment 58 Miles Parker CLA 2013-07-15 12:58:39 EDT
Yeah, Tomek!! :)
Comment 59 Tomasz Zarna CLA 2013-07-15 17:48:21 EDT
The last patch for publish comments [1] not only allows commenting a review, but also makes almost all tests green, with the exception of:
* org.eclipse.mylyn.gerrit.tests.core.client.GerritClientTest.testGetVersion() [2]
* org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactoryTest.testApprovals(), indicating the issue mentioned in comment 57
* org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetRemoteFactoryTest.testPatchSetComments()

In the meantime, I added a test for abandoning a change [3], and even though it passes for 2.5, it's going to fail for 2.6 with "unknown service" exception. I expect more of the Gerrit operations to fail like that, so once I'm done with existing tests, I'm gonna continue adding new (initially failing) tests and fix them one by one.

[1] https://git.eclipse.org/r/#/c/14561/
[2] https://git.eclipse.org/r/#/c/14557/
[3] https://git.eclipse.org/r/#/c/14559/
Comment 60 Sam Davis CLA 2013-07-15 20:10:28 EDT
Tomek, great work!!!
Comment 61 Tomasz Zarna CLA 2013-07-17 16:36:06 EDT
The following are all work-in-progress patches, just to let you know what I'm on:
* failing GerritReviewRemoteFactoryTest.testApprovals() -- https://git.eclipse.org/r/#/c/14589
* failing PatchSetRemoteFactoryTest.testPatchSetComments() -- https://git.eclipse.org/r/14561
* "unknown service" exception when abandoning a change -- https://git.eclipse.org/r/#/c/14625
Comment 62 Tomasz Zarna CLA 2013-07-19 12:33:36 EDT
I've just finished adding a bunch of tests, which pass for 2.5, but fail for 2.6 (mostly with the "unknown service" error) [1]. 

Having them all merged would give us 54 tests _in total_, out of which 21 fail against 2.6. This gives us the pass rate at the level of 62%. My goal is to make it 100% by the end of next week.

[1] http://wiki.eclipse.org/Mylyn/Reviews/Gerrit_2.6#For_2.0.1 / "add more tests" bullet
Comment 63 Miles Parker CLA 2013-07-19 13:16:58 EDT
(In reply to comment #62)
> I've just finished adding a bunch of tests, which pass for 2.5, but fail for 2.6
> (mostly with the "unknown service" error) [1].
> 
> Having them all merged would give us 54 tests _in total_, out of which 21 fail
> against 2.6. This gives us the pass rate at the level of 62%. My goal is to make
> it 100% by the end of next week.

Sounds good Tomek -- I'll look forward to testing when I return.

> 
> [1] http://wiki.eclipse.org/Mylyn/Reviews/Gerrit_2.6#For_2.0.1 / "add more
> tests" bullet
Comment 64 Tomasz Zarna CLA 2013-07-23 15:35:20 EDT
To make reviewing a little bit easier I created a dependency graph of the most important changes I'm working on: http://wiki.eclipse.org/Mylyn/Reviews/Gerrit_2.6#For_2.0.1
Comment 65 Tomasz Zarna CLA 2013-07-24 12:11:44 EDT
With a bunch of changes merged in this morning (thx Steffen), the current status is: 65 tests (51 on master + 14 on changes), out of which 19 (17 + 2) fail against 2.6. Passing rate : *71%*.
Comment 66 Miles Parker CLA 2013-07-29 14:53:18 EDT
(In reply to comment #64)
> To make reviewing a little bit easier I created a dependency graph of the most
> important changes I'm working on:
> http://wiki.eclipse.org/Mylyn/Reviews/Gerrit_2.6#For_2.0.1

Neat! What tool did you use to generate this, Tomek?
Comment 67 Tomasz Zarna CLA 2013-07-29 15:50:05 EDT
(In reply to comment #66)
> Neat! What tool did you use to generate this, Tomek?

http://www.graphviz.org/, I like it so much that I use it more often than a sheet of paper next to my keyboard ;)
Comment 68 Miles Parker CLA 2013-07-29 15:59:45 EDT
(In reply to comment #67)
> (In reply to comment #66)
> > Neat! What tool did you use to generate this, Tomek?
> 
> http://www.graphviz.org/, I like it so much that I use it more often than a
> sheet of paper next to my keyboard ;)

Yeah, I thought it looked familiar -- I've used it for much more complex stuff like this http://metascapeabm.com/models/tb/TBmodel/graphvis/svg/individual__Rules.svg but it never occurred to me to use it for one off diagrams. Nice.
Comment 69 Tomasz Zarna CLA 2013-07-29 16:07:02 EDT
(In reply to comment #68)
> 've used it for much more complex stuff like this
> http://metascapeabm.com/models/tb/TBmodel/graphvis/svg/individual__Rules.svg

If you had left me alone without reviewing my patches for one more week I think I could have gotten close to that ;)
Comment 70 Miles Parker CLA 2013-07-29 18:14:04 EDT
An alternative proposal for https://git.eclipse.org/r/#/c/14915/ (impacts https://git.eclipse.org/r/#/c/14589/)
Comment 71 Miles Parker CLA 2013-07-29 18:14:46 EDT
Forgot the review! https://git.eclipse.org/r/#/c/14949/ (Haven't tested yet..)
Comment 72 Tomasz Zarna CLA 2013-07-30 10:33:35 EDT
Currently, there are 91 tests out of which 5 fail against 2.6. Passing rate: *94%*. With https://git.eclipse.org/r/#/c/14961/ they should all pass.
Comment 73 Steffen Pingel CLA 2013-07-30 13:31:15 EDT
I have merged a number of changes including the latest that was required to make tests pass. We now have an updated snapshot build on the usual weekly update site that should enable all (?) connector features on 2.6. The test flag has been removed from the 2.6 Gerrit repository so integration tests will automatically run against 2.6 from now on.

I only get 1 test failure now on Gerrit 2.4.4 (all tests pass on other repositories including 2.6.1):

java.lang.AssertionError: 
Expected: is "Not Found"
     got: "Change is not abandoned or patchset is not latest"

	at org.junit.Assert.assertThat(Assert.java:778)
	at org.junit.Assert.assertThat(Assert.java:736)
	at org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactoryTest.testRestoreNewChange(GerritReviewRemoteFactoryTest.java:269)
Comment 74 Miles Parker CLA 2013-07-30 13:34:55 EDT
(In reply to comment #73)
> I have merged a number of changes including the latest that was required to make
> tests pass. We now have an updated snapshot build on the usual weekly update
> site that should enable all (?) connector features on 2.6. The test flag has
> been removed from the 2.6 Gerrit repository so integration tests will
> automatically run against 2.6 from now on.

Nice!!

> I only get 1 test failure now on Gerrit 2.4.4 (all tests pass on other
> repositories including 2.6.1):
> 
> java.lang.AssertionError:
> Expected: is "Not Found"
> got: "Change is not abandoned or patchset is not latest"
> 
> at org.junit.Assert.assertThat(Assert.java:778)
> at org.junit.Assert.assertThat(Assert.java:736)
> at
> org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactoryTest.testRestoreNewChange(GerritReviewRemoteFactoryTest.java:269)

Yeah, I just noticed that yesterday when testing locally as well.
Comment 75 Tomasz Zarna CLA 2013-07-30 15:23:44 EDT
(In reply to comment #73)
> org.eclipse.mylyn.internal.gerrit.core.remote.GerritReviewRemoteFactoryTest.testRestoreNewChange(GerritReviewRemoteFactoryTest.java:269)

It's an issue with the test, here is a fix for it: https://git.eclipse.org/r/#/c/14989/
Comment 76 Tomasz Zarna CLA 2013-08-13 08:27:15 EDT
All blockers have been merged, marking this one as fixed. Please open separate bugs in case of any issues with the 2.6 support.
Comment 77 Miles Parker CLA 2013-08-13 12:53:39 EDT
Excellent, Tomek. And thanks for everyone's patience.
Comment 78 Sam Davis CLA 2013-08-13 14:20:57 EDT
Congratulations to everyone involved in making this happen!