Improve Gerrit connector usability #230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it possible to use http password token that is defined in Settings -> HTTP Password page instead of the regular site password. https://gerrit-review.googlesource.com/Documentation/rest-api.html#authentication
When a cherry-pick is done and there are more commits with the same changeId, an exended id can be used to distinguish between them. It has a tilde separated format: project/subproject~branch/subbranch~changeId If this is not encoded but just put in the URL, an error message comes from the server like: Request not successful. Message: Not Found. Status-Code: 404. Content: Not found: project
As it's a Gerrit feature and not a client side deduplication, it should not have a large overhead, therefore it's enabled by default.
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
============================================
+ Coverage 72.38% 74.16% +1.78%
- Complexity 608 625 +17
============================================
Files 146 147 +1
Lines 1988 2009 +21
Branches 131 131
============================================
+ Hits 1439 1490 +51
+ Misses 489 457 -32
- Partials 60 62 +2
Continue to review full report at Codecov.
|
SpOOnman
reviewed
Jan 21, 2021
| } | ||
| // To keep the changeId readable, we don't encode '~' (it is not needed according to RFC2396) | ||
| return StreamSupport.stream(Splitter.on('~').split(changeId).spliterator(), false) | ||
| .map(GerritFacadeBuilder::safeUrlEncode).collect(Collectors.joining("~")); |
There was a problem hiding this comment.
I just hope you're right and it works :D
There was a problem hiding this comment.
I will add a comment with a reference to
ChangesRestClient.id where they do it in a very similar way with the 2 and 3 argument methods.
| } | ||
|
|
||
| @Test | ||
| void build_shouldEscapeChangeIdWithSlash() { |
There was a problem hiding this comment.
Just to fit project style please skip build_ prefix
| void build_shouldEscapeChangeIdWithSlash() { | ||
| GerritFacade connector = gerritFacadeBuilder.build(configuration); | ||
|
|
||
| assertEquals("project%2Fsubproject~branch%2Fsubbranch~changeId", connector.gerritPatchset.getChangeId()); |
There was a problem hiding this comment.
Please use assertj assertions
| } | ||
|
|
||
| @Test | ||
| void build_optionsPassed() { |
There was a problem hiding this comment.
shouldBuildOptions maybe?
| @@ -56,7 +67,38 @@ void shouldNotListDeletedFiles() throws IOException, RestApiException { | |||
| assertThat(reviewFiles).hasSize(1); | |||
| } | |||
|
|
|||
| @Test | |||
| void publish() throws IOException, RestApiException { | |||
There was a problem hiding this comment.
Please use should... naming style
added 2 commits
January 22, 2021 07:28
This actually does the same as the old 'safeUrlEncode(String)'. Also a reference is now added for the ID encoding method.
|
@SpOOnman Please check the changes again, I think I considered all your comments. |
SpOOnman
approved these changes
Jan 28, 2021
| // To keep the changeId readable, we don't encode '~' (it is not needed according to RFC2396) | ||
| // See also: ChangesRestClient.id(String, String) and ChangesRestClient.id(String, String, String) | ||
| return StreamSupport.stream(Splitter.on('~').split(changeId).spliterator(), false) | ||
| .map(Url::encode).collect(Collectors.joining("~")); |
This was referenced Jan 28, 2021
rufuslevi
pushed a commit
to rufuslevi/sputnik
that referenced
this pull request
Mar 12, 2024
* Allow usage of Gerrit HTTP password tokens (TouK#220) This makes it possible to use http password token that is defined in Settings -> HTTP Password page instead of the regular site password. https://gerrit-review.googlesource.com/Documentation/rest-api.html#authentication * URL encode changeId when creating GerritPatchset (TouK#221) When a cherry-pick is done and there are more commits with the same changeId, an exended id can be used to distinguish between them. It has a tilde separated format: project/subproject~branch/subbranch~changeId If this is not encoded but just put in the URL, an error message comes from the server like: Request not successful. Message: Not Found. Status-Code: 404. Content: Not found: project * Support gerrit configuration to omit duplicate comments (TouK#108) As it's a Gerrit feature and not a client side deduplication, it should not have a large overhead, therefore it's enabled by default. * Improve test coverage of Gerrit connector * Use Gerrit API's URL encoding in GerritFacadeBuilder This actually does the same as the old 'safeUrlEncode(String)'. Also a reference is now added for the ID encoding method. * Clean up Gerrit connector tests to better fit the project style Co-authored-by: Gabor Garancsi <gabor.garancsi@casrd.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow usage of Gerrit HTTP password tokens (#220)
URL encode changeId when creating GerritPatchset (#221)
Support gerrit configuration to omit duplicate comments (#108)