Notice: This Wiki is now read only and edits are no longer possible. Please see: https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/wikis/Wiki-shutdown-plan for the plan.
Difference between revisions of "Mylyn/Reviews/Gerrit 2.6"
m (→What needs to be done) |
m (→What needs to be done) |
||
Line 69: | Line 69: | ||
* ... same for JSON objects sent in requests | * ... same for JSON objects sent in requests | ||
* avoid exception driven logic in GerritClient | * avoid exception driven logic in GerritClient | ||
− | * [[Image:Progress.gif]] decouple Remote API from GerritClient, allowing to provide different implementations depending on the targeted Gerrit version | + | * [[Image:Progress.gif]] decouple Remote API from GerritClient, allowing to provide different implementations depending on the targeted Gerrit version, https://git.eclipse.org/r/#/c/14229/ |
* provide those implementations | * provide those implementations | ||
* [[Image:Glass.gif]] not sure if it makes much sense, as it's basically testing Gerrit not Reviews, but we could consider adding some simple tests parsing JSON responses (to detect breaking changes early) | * [[Image:Glass.gif]] not sure if it makes much sense, as it's basically testing Gerrit not Reviews, but we could consider adding some simple tests parsing JSON responses (to detect breaking changes early) | ||
* ... | * ... |
Revision as of 12:29, 3 July 2013
Here are my findings from trying to setup Gerrit 2.6 with Mylyn Reviews (while tackling bug 395059)
Contents
Glimpse at the code
- The connector already uses some REST API, e.g. for querying changes
- Form based login (GET /login/mine with creds set) still works, the login cookie seems to be properly set. The client is asked to redirect (302), but it looks this can be safely ignored. Providing invalid creds results in an auth failure, good.
- GerritClient (used for calling RPC API) expects JSON responses: see
org.eclipse.mylyn.internal.gerrit.core.client.GerritService.invoke(Object, Method, Object[])
- Current support for different Gerrit versions:
- there are places where GerritClient fallbacks to REST e.g. calling
GerritClient#executeQueryRest
fromGerritClient#queryMyReviews
- OTOH, there is a fallback to Gerrit <=2.2.1 in
GerritClient#getVisibleProjects(IProgressMonitor, GerritConfig)
- it looks like the client cannot decide if fallbacks should be used for supporting newer or older versions of Gerrit
- there are places where GerritClient fallbacks to REST e.g. calling
Running tests
- Some tests fail because getVisibleProjects (RPC) gets
"Invalid xsrfKey in request"
response while refreshing repo config - Tests from
GerritReviewRemoteFactoryTest
andPatchSetRemoteFactoryTest
fail waiting for a response
Using the connector with Gerrit 2.6
- validating repo as anonymous:
{"jsonrpc":"2.0","id":1,"error":{"code":-32603,"message":"No such service method"}}
- getting configuration before creating a new query: the client tries to parse an HTML page in
GerritClient.refreshGerritConfig(IProgressMonitor)
, and fails as it gets a 302 page in Gerrit 2.6 - if I ignore the error about config not available I'm able to create a query but get an NPE (quick fixed in https://git.eclipse.org/r/#/c/13982/)
- ^^^ should I be able to do that in the first place?
Comments on Gerrit 2.6 Release Notes
Incompatibilities introduced by the new REST API:
"The structure of the AuditEvent has been extended to support the new name-multivalue pairs used in the REST API. This is breaking compatibility with the 2.5 API as it changes the params data type, this is needed anyway as the previous list of Objects was not providing all the necessary information of "what relates to what" in terms of parameters info."
"Remove support for deprecated --format option when listing changes. Querying changes via REST is now always producing JSON output."
The latter has been already fixed in https://git.eclipse.org/r/#/c/14181/
More info: http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.6.html#_rest_api
ID changes, mentioned by Shawn on bug 395059:
- Changes: http://gerrit-documentation.googlecode.com/svn/Documentation/2.6/rest-api-changes.html#ids
- Projects: http://gerrit-documentation.googlecode.com/svn/Documentation/2.6/rest-api-projects.html#ids
- not sure if we care much about Groups and Accounts
I think it's safe to assume that RPC API should still be operational and it's the REST API (urls and JSON objects) that needs to be taken care of. However, my guess is that the RPC interface is going to be dropped once the REST API is stable, so we should be using it wherever possible.
Draft review
...with quick and dirty fixes https://git.eclipse.org/r/#/c/13982/
Applying the changes gives a semi-functional connector: I'm able to create a Gerrit 2.6 repo as anonymous and add a query. I can even open a review editor with some UI bits properly populated: e.g. can see Change Sets and files in them. There's still a bunch of NPE flying around and authentication looks totally broken.
What the patch covers:
- switches RPC interface from
/gerrit/rpc
to/gerrit_ui/rpc
(no backward compatibility!) -
runs queries even whenalready fixed in https://git.eclipse.org/r/#/c/14181/format
paramater is not supported - avoids NPE in GerritReviewRemoteFactory since gerrit config cannot be retrieved
- turns
testPerformQueryAnonymous
andtestGetAccountAnonymous
tests green
What needs to be done
- Fix authentication
- remote usage of deprecated
format
query parameter, fixed in https://git.eclipse.org/r/#/c/14181/ - find out the Gerrit version (was: Parsing HTML and getting Gerrit config from HTML page no longer works in 2.6), here is a better way of doing this, thx Shawn
- Update expected JSON objects in responses
- ... same for JSON objects sent in requests
- avoid exception driven logic in GerritClient
- decouple Remote API from GerritClient, allowing to provide different implementations depending on the targeted Gerrit version, https://git.eclipse.org/r/#/c/14229/
- provide those implementations
- not sure if it makes much sense, as it's basically testing Gerrit not Reviews, but we could consider adding some simple tests parsing JSON responses (to detect breaking changes early)
- ...