Skip to content
This repository has been archived by the owner on Sep 12, 2024. It is now read-only.

Commit

Permalink
Adding global flag --strict-start to wildcard-based commands.
Browse files Browse the repository at this point in the history
This forces job names to be matched from the string start. By default,
some commands will match on any jobs that contain the input name as a
substring, this option forces the match to only happen if the string
starts with the input name. Affects the subcommands: remove, inspect,
rolling-update, undeploy, deploy and stop.

The motivation behind this is production usages returning
"JOB_AMBIGUOUS_REFERENCE" in cases where a job's name is a substring of
another (eg: foo-bar and bar-foo-bar, as the first a substring of the
second).
  • Loading branch information
hstefan authored and Stefan Puhlmann committed Apr 9, 2019
1 parent c9000bc commit 22ab3e1
Show file tree
Hide file tree
Showing 8 changed files with 63 additions and 6 deletions.
11 changes: 11 additions & 0 deletions helios-tools/src/main/java/com/spotify/helios/cli/CliParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ public CliParser(final String... args)
this.loggingConfig = new LoggingConfig(options.getInt(globalArgs.verbose.getDest()),
false, null,
options.getBoolean(globalArgs.noLogSetup.getDest()));
this.strictStart = options.getBoolean(globalArgs.strictJobStart.getDest());

// Merge domains and explicit endpoints into master endpoints
final List<String> explicitEndpoints = options.getList(globalArgs.masterArg.getDest());
Expand Down Expand Up @@ -268,6 +269,7 @@ private static class GlobalArgs {
private final Argument verbose;
private final Argument noLogSetup;
private final Argument jsonArg;
private final Argument strictJobStart;
private final ArgumentGroup globalArgs;
private final boolean topLevel;

Expand Down Expand Up @@ -317,6 +319,15 @@ private static class GlobalArgs {
.action(storeTrue())
.help(SUPPRESS);

strictJobStart = addArgument("-s", "--strict-start")
.type(Boolean.class)
.setDefault(false)
.help("Forces job names to be matched from the string start. "
+ "By default, some commands will match on any jobs that contain the input name as a"
+ " substring, this option forces the match to only happen if the string starts with"
+ " the input name. Affects the subcommands: remove, inspect, rolling-update,"
+ " undeploy, deploy and stop.");

// note: because of the way the HeliosClient is constructed, these next arguments are
// read indirectly in cli/Utils.java:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ private static String formatRolloutOptions(final RolloutOptions options) {
}

public JobInspectCommand(final Subparser parser) {
super(parser);
super(parser, false);
parser.help("print the configuration of a job");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class JobRemoveCommand extends WildcardJobCommand {
private final Argument yesArg;

public JobRemoveCommand(Subparser parser) {
super(parser);
super(parser, false);

parser.help("remove a job");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class JobStartCommand extends WildcardJobCommand {
private final Argument tokenArg;

public JobStartCommand(Subparser parser) {
super(parser);
super(parser, false);

parser.help("start a stopped job");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class JobStopCommand extends WildcardJobCommand {
private final Argument tokenArg;

public JobStopCommand(Subparser parser) {
super(parser);
super(parser, false);

parser.help("stop a running job without undeploying it");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class JobUndeployCommand extends WildcardJobCommand {
private final Argument yesArg;

public JobUndeployCommand(final Subparser parser) {
super(parser);
super(parser, false);

parser.help("undeploy a job from hosts");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
abstract class WildcardJobCommand extends ControlCommand {

private final Argument jobArg;
private final Argument strictStartArg;

public WildcardJobCommand(final Subparser parser) {
this(parser, false);
Expand All @@ -51,16 +52,31 @@ public WildcardJobCommand(final Subparser parser, final boolean shortCircuit) {

jobArg = parser.addArgument("job")
.help("Job id.");
strictStartArg = parser.addArgument("--strict-start")
.type(Boolean.class)
.setDefault(false)
.help("Forces job names to be matched from the string start. "
+ "By default, some commands will match on any jobs that contain the input name as a"
+ " substring, this option forces the match to only happen if the string starts with"
+ " the input name. Affects the subcommands: remove, inspect, rolling-update,"
+ " undeploy, deploy and stop.");
}

@Override
int run(final Namespace options, final HeliosClient client, final PrintStream out,
final boolean json, final BufferedReader stdin)
throws ExecutionException, InterruptedException, IOException {

final String jobIdString = options.getString(jobArg.getDest());
final Map<JobId, Job> jobs = client.jobs(jobIdString).get();

if (options.getBoolean(strictStartArg.getDest())) {
for (final Map.Entry<JobId, Job> entry : jobs.entrySet()) {
if (!entry.getKey().toShortString().startsWith(jobIdString)) {
jobs.remove(entry.getKey());
}
}
}

if (jobs.size() == 0) {
if (!json) {
out.printf("Unknown job: %s%n", jobIdString);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static com.spotify.helios.common.descriptors.DeploymentGroup.RollingUpdateReason.MANUAL;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.mockito.Matchers.any;
Expand Down Expand Up @@ -648,6 +649,35 @@ public void testTimeoutDuringRolloutOutput() throws Exception {
assertThat(baos.toString(), containsString(expectedSubstring));
}

@Test
public void testStrictStartJobName() throws Exception {
final JobId jobId1 = JobId.parse("foo-bar:cafe");
Job job1 = Job.newBuilder()
.setName(jobId1.getName())
.setHash(jobId1.getHash())
.setRolloutOptions(jobOptions)
.build();
final JobId jobId2 = JobId.parse("bar:beef");
Job job2 = Job.newBuilder()
.setName(jobId2.getName())
.setHash(jobId2.getHash())
.setRolloutOptions(jobOptions)
.build();
final Map<JobId, Job> jobs = ImmutableMap.of(
jobId1, job1,
jobId2, job2
);
when(client.jobs(anyString())).thenReturn(immediateFuture(jobs));

when(options.getBoolean("strict-start")).thenReturn(true);

final int ret = command.run(options, client, out, false, null);

assertEquals(ret, 0);

assertThat(out.toString(), not(containsString("Ambiguous job reference")));
}

private static class TimeUtil implements RollingUpdateCommand.SleepFunction, Supplier<Long> {

private long curentTimeMillis = 0;
Expand Down

0 comments on commit 22ab3e1

Please sign in to comment.