Remove replication option from ssh command
Ssh rename-project command does not require replication option to
perform rename. Gerrit config contains isReplica flag, which do
the same thing as replication option for ssh command. Meaning
replication option no longer needed.
Change-Id: I335695c5e94d8f6a76bc8e06d47cd670dcbe2150
diff --git a/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameCommand.java b/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameCommand.java
index cac0b30..3e2c796 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameCommand.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameCommand.java
@@ -14,12 +14,9 @@
package com.googlesource.gerrit.plugins.renameproject;
-import static com.googlesource.gerrit.plugins.renameproject.RenameProject.CANCELLATION_MSG;
import static com.googlesource.gerrit.plugins.renameproject.RenameProject.WARNING_LIMIT;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.project.ProjectResource;
@@ -37,7 +34,6 @@
import java.util.NoSuchElementException;
import java.util.Optional;
import org.kohsuke.args4j.Argument;
-import org.kohsuke.args4j.Option;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -49,9 +45,6 @@
@Argument(index = 1, required = true, metaVar = "NEWNAME", usage = "new name for the project")
private String newProjectName;
- @Option(name = "--replication", usage = "perform only file system rename")
- private boolean replication;
-
private static final Logger log = LoggerFactory.getLogger(RenameCommand.class);
private final RenameProject renameProject;
private final Provider<CurrentUser> self;
@@ -68,25 +61,13 @@
RenameProject.Input input = new RenameProject.Input();
input.name = newProjectName;
ProjectResource rsrc = new ProjectResource(projectState, self.get());
-
- if (replication) {
- if (renameProject.isAdmin()) {
- renameProject.fsRenameStep(
- rsrc.getNameKey(), Project.nameKey(newProjectName), Optional.empty());
- } else {
- throw new AuthException("Not allowed to replicate rename");
- }
- } else {
- try (CommandProgressMonitor monitor = new CommandProgressMonitor(stdout)) {
- renameProject.assertCanRename(rsrc, input, Optional.of(monitor));
- List<Change.Id> changeIds = renameProject.getChanges(rsrc, Optional.of(monitor));
- if (continueRename(changeIds, monitor)) {
- renameProject.doRename(changeIds, rsrc, input, Optional.of(monitor));
- } else {
- log.debug(CANCELLATION_MSG);
- stdout.println(CANCELLATION_MSG);
- stdout.flush();
- }
+ try (CommandProgressMonitor monitor = new CommandProgressMonitor(stdout)) {
+ Optional<ProgressMonitor> optionalProgressMonitor = Optional.of(monitor);
+ renameProject.assertCanRename(rsrc, input, optionalProgressMonitor);
+ List<Change.Id> changeIds = renameProject.getChanges(rsrc, optionalProgressMonitor);
+ if (!renameProject.startRename(
+ rsrc, input, Optional.of(monitor), continueRename(changeIds, monitor), changeIds)) {
+ stdout.flush();
}
}
} catch (NoSuchElementException | RestApiException | IOException e) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameProject.java b/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameProject.java
index 6ec97c9..19d5b71 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameProject.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/renameproject/RenameProject.java
@@ -72,27 +72,39 @@
public Response<?> apply(ProjectResource resource, Input input)
throws IOException, AuthException, BadRequestException, ResourceConflictException,
InterruptedException, ConfigInvalidException, RenameRevertException {
- if (!isReplica) {
- assertCanRename(resource, input, Optional.empty());
- return renameAction(resource, input);
+ Optional<ProgressMonitor> optionalProgressMonitor = Optional.empty();
+ assertCanRename(resource, input, optionalProgressMonitor);
+ List<Id> changeIds = getChanges(resource, optionalProgressMonitor);
+ if (startRename(
+ resource,
+ input,
+ Optional.empty(),
+ (changeIds == null || changeIds.size() <= WARNING_LIMIT || input.continueWithRename),
+ changeIds)) {
+ return Response.ok("");
}
- if (isAdmin()) {
- return renameAction(resource, input);
- }
- throw new AuthException("You do not have enough privileges for this operation");
+ return Response.none();
}
- private Response<?> renameAction(ProjectResource resource, Input input)
- throws IOException, ConfigInvalidException, RenameRevertException, InterruptedException {
- List<Id> changeIds = getChanges(resource, Optional.empty());
-
- if (changeIds == null || changeIds.size() <= WARNING_LIMIT || input.continueWithRename) {
- doRename(changeIds, resource, input, Optional.empty());
+ public boolean startRename(
+ ProjectResource resource,
+ Input input,
+ Optional<ProgressMonitor> optionalProgressMonitor,
+ boolean continueRename,
+ List<Change.Id> changeIds)
+ throws ResourceConflictException, BadRequestException, AuthException, IOException,
+ ConfigInvalidException, RenameRevertException, InterruptedException {
+ if (!isReplica) {
+ if (continueRename) {
+ doRename(Optional.ofNullable(changeIds), resource, input, optionalProgressMonitor);
+ } else {
+ log.debug(CANCELLATION_MSG);
+ return false;
+ }
} else {
- log.debug(CANCELLATION_MSG);
- return Response.none();
+ doRename(Optional.empty(), resource, input, Optional.empty());
}
- return Response.ok("");
+ return true;
}
public static class Input {
@@ -185,7 +197,7 @@
}
private void assertRenamePermission(ProjectResource rsrc) throws AuthException {
- if (!canRename(rsrc)) {
+ if ((isReplica && !isAdmin()) || !canRename(rsrc)) {
throw new AuthException("Not allowed to rename project");
}
}
@@ -222,15 +234,16 @@
return true;
}
- void assertCanRename(ProjectResource rsrc, Input input, Optional<ProgressMonitor> pm)
+ void assertCanRename(ProjectResource rsrc, Input input, Optional<ProgressMonitor> opm)
throws ResourceConflictException, BadRequestException, AuthException {
try {
- pm.ifPresent(progressMonitor -> progressMonitor.beginTask("Checking preconditions"));
-
+ opm.ifPresent(progressMonitor -> progressMonitor.beginTask("Checking preconditions"));
assertNewNameNotNull(input);
assertNewNameMatchesRegex(input);
assertRenamePermission(rsrc);
- renamePreconditions.assertCanRename(rsrc, Project.nameKey(input.name));
+ if (!isReplica) {
+ renamePreconditions.assertCanRename(rsrc, Project.nameKey(input.name));
+ }
log.debug("Rename preconditions check successful.");
} catch (CannotRenameProjectException e) {
throw new ResourceConflictException(e.getMessage());
@@ -238,36 +251,39 @@
}
void doRename(
- List<Change.Id> changeIds, ProjectResource rsrc, Input input, Optional<ProgressMonitor> pm)
+ Optional<List<Change.Id>> opChangeIds,
+ ProjectResource rsrc,
+ Input input,
+ Optional<ProgressMonitor> opm)
throws InterruptedException, ConfigInvalidException, IOException, RenameRevertException {
Project.NameKey oldProjectKey = rsrc.getNameKey();
Project.NameKey newProjectKey = Project.nameKey(input.name);
Exception ex = null;
stepsPerformed.clear();
try {
- fsRenameStep(oldProjectKey, newProjectKey, pm);
+ fsRenameStep(oldProjectKey, newProjectKey, opm);
if (!isReplica) {
cacheRenameStep(rsrc.getNameKey(), newProjectKey);
List<Change.Id> updatedChangeIds =
- dbRenameStep(changeIds, oldProjectKey, newProjectKey, pm);
+ dbRenameStep(opChangeIds.get(), oldProjectKey, newProjectKey, opm);
// if the DB update is successful, update the secondary index
- indexRenameStep(updatedChangeIds, oldProjectKey, newProjectKey, pm);
+ indexRenameStep(updatedChangeIds, oldProjectKey, newProjectKey, opm);
// no need to revert this since newProjectKey will be removed from project cache before
lockUnlockProject.unlock(newProjectKey);
log.debug("Unlocked the repo {} after rename operation.", newProjectKey.get());
// flush old changeId -> Project cache for given changeIds
- changeIdProjectCache.invalidateAll(changeIds);
+ changeIdProjectCache.invalidateAll(opChangeIds.get());
pluginEvent.fire(pluginName, pluginName, oldProjectKey.get() + ":" + newProjectKey.get());
// replicate rename-project operation to other replica instances
- replicateRename(sshHelper, httpSession, input, oldProjectKey, pm);
+ replicateRename(sshHelper, httpSession, input, oldProjectKey, opm);
}
} catch (Exception e) {
if (stepsPerformed.isEmpty()) {
@@ -280,7 +296,7 @@
}
try {
revertRenameProject.performRevert(
- stepsPerformed, changeIds, oldProjectKey, newProjectKey, pm);
+ stepsPerformed, opChangeIds.get(), oldProjectKey, newProjectKey, opm);
} catch (Exception revertEx) {
log.error(
"Failed to revert renaming procedure for {}. Exception caught: {}",
@@ -297,9 +313,9 @@
}
void fsRenameStep(
- Project.NameKey oldProjectKey, Project.NameKey newProjectKey, Optional<ProgressMonitor> pm)
+ Project.NameKey oldProjectKey, Project.NameKey newProjectKey, Optional<ProgressMonitor> opm)
throws IOException {
- fsHandler.rename(oldProjectKey, newProjectKey, pm);
+ fsHandler.rename(oldProjectKey, newProjectKey, opm);
logPerformedStep(Step.FILESYSTEM, newProjectKey, oldProjectKey);
}
@@ -313,9 +329,9 @@
List<Change.Id> changeIds,
Project.NameKey oldProjectKey,
Project.NameKey newProjectKey,
- Optional<ProgressMonitor> pm)
+ Optional<ProgressMonitor> opm)
throws IOException, ConfigInvalidException, RenameRevertException {
- List<Change.Id> updatedChangeIds = dbHandler.rename(changeIds, newProjectKey, pm);
+ List<Change.Id> updatedChangeIds = dbHandler.rename(changeIds, newProjectKey, opm);
logPerformedStep(Step.DATABASE, newProjectKey, oldProjectKey);
return updatedChangeIds;
}
@@ -324,9 +340,9 @@
List<Change.Id> updatedChangeIds,
Project.NameKey oldProjectKey,
Project.NameKey newProjectKey,
- Optional<ProgressMonitor> pm)
+ Optional<ProgressMonitor> opm)
throws InterruptedException {
- indexHandler.updateIndex(updatedChangeIds, newProjectKey, pm);
+ indexHandler.updateIndex(updatedChangeIds, newProjectKey, opm);
logPerformedStep(Step.INDEX, newProjectKey, oldProjectKey);
}
@@ -400,9 +416,7 @@
throws RenameReplicationException, URISyntaxException, IOException {
OutputStream errStream = sshHelper.newErrorBufferStream();
sshHelper.executeRemoteSsh(
- new URIish(url),
- pluginName + " " + oldProjectKey.get() + " " + input.name + " --replication",
- errStream);
+ new URIish(url), pluginName + " " + oldProjectKey.get() + " " + input.name, errStream);
String errorMessage = errStream.toString();
if (!errorMessage.isEmpty()) {
throw new RenameReplicationException(errorMessage);
@@ -450,6 +464,7 @@
"Rescheduling a rename replication for retry for {} on project {}",
url,
oldProjectKey.get());
+ e.printStackTrace();
failedReplicas.add(url);
}
}
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md
index 80ef373..52431c7 100644
--- a/src/main/resources/Documentation/about.md
+++ b/src/main/resources/Documentation/about.md
@@ -66,6 +66,3 @@
is granted the 'Rename Own Project' capability (provided by this
plugin). However, because of all the caveats of this plugin, it is not
recommended to delegate the 'Rename Project' capability to any non-admin user.
-
-As previously described, this plugin is capable of performing only file system rename, if a user is
-an admin user and if the `--replication` option is used.
diff --git a/src/main/resources/Documentation/cmd-rename.md b/src/main/resources/Documentation/cmd-rename.md
index 0f54eb6..08c6974 100644
--- a/src/main/resources/Documentation/cmd-rename.md
+++ b/src/main/resources/Documentation/cmd-rename.md
@@ -11,7 +11,6 @@
ssh -p @SSH_PORT@ @SSH_HOST@ @PLUGIN@
<PROJECT>
<NEWNAME>
- [--replication]
```
DESCRIPTION
@@ -29,15 +28,6 @@
---------
This command is intended to be used in scripts.
-
-OPTIONS
--------
-`--replication`
-: To perform only file system rename. This option is used for replication of
- rename operation to other replica instances. This command should not be used
- towards non-replica instances. This option requires the user to have admin
- permissions.
-
EXAMPLES
--------
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 2be087c..375bdd1 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -25,47 +25,47 @@
```
The plugin supports both http and ssh replication.
-To configure ssh replication specify the port number, and it is required to put the `ssh://` prefix followed by hostname and then
-port number after `:`. It is also possible to specify the ssh user by passing `USERNAME@` as a
-prefix for hostname.
+To configure ssh replication, specify the port number and put the `ssh://`
+prefix followed by hostname and then port number after `:`. It is also possible
+to specify the ssh user by passing `USERNAME@` as a prefix for hostname.
-Rename replication is done over SSH, so ensure the host key of the remote system(s) is already in
-the Gerrit user's `~/.ssh/known_hosts` file. The easiest way to add the host key is to connect once
-by hand with the command line:
+Rename replication is done over SSH, so ensure the host key of the remote
+system(s) is already in the Gerrit user's `~/.ssh/known_hosts` file. The easiest
+way to add the host key is to connect once by hand with the command line:
```
sudo su -c 'ssh mirror1.us.some.org echo' gerrit2
```
-@PLUGIN@ plugin uses the ssh rename command towards the replica(s) with `--replication` option to
-replicate the rename operation. It is possible to customize the parameters of the underlying ssh
-client doing these calls by specifying the following fields:
-* `sshCommandTimeout` : Timeout for SSH command execution. If 0, there is no timeout, and
- the client waits indefinitely. By default, 0.
-* `sshConnectionTimeout` : Timeout for SSH connections in minutes. If 0, there is no timeout, and
- the client waits indefinitely. By default, 2 minutes.
+It is possible to customize the parameters of the underlying ssh client doing
+these calls by specifying the following fields:
+* `sshCommandTimeout` : Timeout for SSH command execution. If 0, there is no
+timeout, and the client waits indefinitely. By default, 0.
+* `sshConnectionTimeout` : Timeout for SSH connections in minutes. If 0, there
+is no timeout, and the client waits indefinitely. By default, 2 minutes.
-To configure http replication, provide the correct url. To cpecify username and password for replication for rename, add
-password and username in gerrit.config or secure.config.
-for example:
+To configure http replication, provide the correct url. To cpecify username and
+password for replication for rename, add password and username in gerrit.config
+or secure.config. for example:
```
[plugin "@PLUGIN@"]
user = username
password = userpassword
```
-Provides a configuration to customize the number of rename replication retries. By default, 3.
+Provides a configuration to customize the number of rename replication retries.
+By default, 3.
```
renameReplicationRetries = 6
```
-Also, this plugin offers a way to restrict the new names of the projects to match an optionally
-configured regex. For example:
+Also, this plugin offers a way to restrict the new names of the projects to
+match an optionally configured regex. For example:
```
[plugin "@PLUGIN@"]
renameRegex = [a-z0-9]+
```
-In this example the new names for projects will be restricted to only non-capital letters and
-numbers.
+In this example the new names for projects will be restricted to only
+non-capital letters and numbers.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/renameproject/RenameIT.java b/src/test/java/com/googlesource/gerrit/plugins/renameproject/RenameIT.java
index 54bf14a..28fe80b 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/renameproject/RenameIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/renameproject/RenameIT.java
@@ -24,7 +24,6 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
-
import com.google.common.base.Joiner;
import com.google.common.cache.Cache;
import com.google.common.net.MediaType;
@@ -50,8 +49,10 @@
import java.util.List;
import java.util.Optional;
import javax.inject.Named;
+import org.apache.http.HttpStatus;
import org.apache.http.auth.AuthenticationException;
import org.apache.http.auth.UsernamePasswordCredentials;
+import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpRequestBase;
import org.apache.http.entity.StringEntity;
@@ -75,7 +76,6 @@
private static final String NEW_PROJECT_NAME = "newProject";
private static final String NON_EXISTING_NAME = "nonExistingProject";
private static final String CACHE_NAME = "changeid_project";
- private static final String REPLICATION_OPTION = "--replication";
private static final String URL = "ssh://localhost:29418";
private static final String RENAME_REGEX = "[a-zA-Z]+";
@@ -102,19 +102,17 @@
@UseLocalDisk
public void testRenameReplicationViaSshNotAdminUser() throws Exception {
createChange();
- userSshSession.exec(
- PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME + " " + REPLICATION_OPTION);
+ userSshSession.exec(PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME);
userSshSession.assertFailure();
- assertThat(userSshSession.getError()).contains("Not allowed to replicate rename");
+ assertThat(userSshSession.getError()).contains("Not allowed to rename project");
}
@Test
@UseLocalDisk
public void testRenameReplicationViaSshAdminUser() throws Exception {
createChange();
- adminSshSession.exec(
- PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME + " " + REPLICATION_OPTION);
+ adminSshSession.exec(PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME);
adminSshSession.assertSuccess();
Optional<ProjectState> projectState = projectCache.get(Project.nameKey(NEW_PROJECT_NAME));
@@ -156,8 +154,7 @@
@UseLocalDisk
public void testRenameReplicationViaSshOnNonExisting() throws Exception {
createChange();
- adminSshSession.exec(
- PLUGIN_NAME + " " + NON_EXISTING_NAME + " " + project.get() + " " + REPLICATION_OPTION);
+ adminSshSession.exec(PLUGIN_NAME + " " + NON_EXISTING_NAME + " " + project.get());
assertThat(adminSshSession.getError()).contains("project " + NON_EXISTING_NAME + " not found");
adminSshSession.assertFailure();
@@ -238,8 +235,7 @@
OutputStream errStream = mock(OutputStream.class);
Input input = new Input();
input.name = NEW_PROJECT_NAME;
- String expectedCommand =
- PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME + " " + REPLICATION_OPTION;
+ String expectedCommand = PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME;
when(sshHelper.newErrorBufferStream()).thenReturn(errStream);
when(errStream.toString()).thenReturn("");
@@ -264,8 +260,7 @@
input.name = NEW_PROJECT_NAME;
when(sshHelper.connect(eq(urish))).thenReturn(session);
renameProject.replicateRename(sshHelper, httpSession, input, project, Optional.empty());
- String expectedCommand =
- PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME + " " + REPLICATION_OPTION;
+ String expectedCommand = PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME;
verify(sshHelper, times(3)).executeRemoteSsh(eq(urish), eq(expectedCommand), eq(errStream));
}
@@ -279,8 +274,7 @@
Input input = new Input();
input.name = NEW_PROJECT_NAME;
- String expectedCommand =
- PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME + " " + REPLICATION_OPTION;
+ String expectedCommand = PLUGIN_NAME + " " + project.get() + " " + NEW_PROJECT_NAME;
when(sshHelper.newErrorBufferStream()).thenReturn(errStream);
when(errStream.toString()).thenReturn("");
@@ -391,18 +385,23 @@
putRequest.setHeader("Content-type", "application/json");
putRequest.setEntity(new StringEntity(body));
try {
- executeRequest(putRequest);
- } catch (RestApiException restApiException) {
- return false;
- } catch (IOException e) {
+ int code = executeRequest(putRequest);
+ if (code != HttpStatus.SC_OK && code != HttpStatus.SC_CREATED) {
+ return false;
+ } else {
+ return true;
+ }
+ } catch (RestApiException | IOException e) {
e.printStackTrace();
}
- return true;
+ return false;
}
- private void executeRequest(HttpRequestBase request) throws IOException, RestApiException {
+ private int executeRequest(HttpRequestBase request) throws IOException, RestApiException {
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
- client.execute(request);
+ CloseableHttpResponse response = client.execute(request);
+ int code = response.getStatusLine().getStatusCode();
+ return code;
} catch (IOException e) {
e.printStackTrace();
throw e;
