Skip to content

Commit

Permalink
Fix #174 Warn if HEALTHCHECK has flags but no arguments
Browse files Browse the repository at this point in the history 
Signed-off-by: Remy Suen <[email protected]>
  • Loading branch information
@rcjsuen
rcjsuen committed Sep 14, 2017
1 parent a8186b4 commit 3fbcf38
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 56 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ All notable changes to this project will be documented in this file.
- HEALTHCHECK's CMD and NONE arguments ([#169](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/169))
- textDocument/publishDiagnostics
- warn if HEALTHCHECK's argument is not CMD or NONE ([#173](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/173))
- warn if HEALTHCHECK has a flag but no arguments ([#174](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/174))

### Fixed
- correct the documentation of HEALTHCHECK's --retries flag's completion item ([#170](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/170))
Expand Down
104 changes: 55 additions & 49 deletions src/dockerValidator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export enum ValidationCode {
ARGUMENT_MISSING,
ARGUMENT_EXTRA,
ARGUMENT_REQUIRES_ONE,
ARGUMENT_REQUIRES_AT_LEAST_ONE,
ARGUMENT_REQUIRES_TWO,
ARGUMENT_REQUIRES_AT_LEAST_TWO,
ARGUMENT_REQUIRES_ONE_OR_THREE,
Expand Down Expand Up @@ -353,64 +354,60 @@ export class Validator {
case "HEALTHCHECK":
let args = instruction.getArguments();
const healthcheckFlags = (instruction as ModifiableInstruction).getFlags();
if (args.length === 0 && healthcheckFlags.length === 0) {
if (args.length === 0) {
// all instructions are expected to have at least one argument
let range = instruction.getInstructionRange();
problems.push(Validator.createMissingArgument(range.start, range.end));
problems.push(Validator.createHEALTHCHECKRequiresAtLeastOneArgument(instruction.getInstructionRange()));
} else {
// check the first argument
if (args.length > 0) {
let value = args[0].getValue();
let uppercase = value.toUpperCase();
if (uppercase === "NONE") {
// check that NONE doesn't have any arguments after it
if (args.length > 1) {
// get the next argument
let start = args[1].getRange().start;
// get the last argument
let end = args[args.length - 1].getRange().end;
// highlight everything after the NONE and warn the user
problems.push(Validator.createHealthcheckNoneUnnecessaryArgument(start, end));
}
// don't need to validate flags of a NONE
break;
} else if (uppercase === "CMD") {
if (args.length === 1) {
// this HEALTHCHECK has a CMD with no arguments
let range = args[0].getRange();
problems.push(Validator.createHealthcheckCmdArgumentMissing(range.start, range.end));
}
} else {
// unknown HEALTHCHECK type
problems.push(Validator.createHealthcheckTypeUnknown(args[0].getRange(), uppercase));
const value = args[0].getValue();
const uppercase = value.toUpperCase();
if (uppercase === "NONE") {
// check that NONE doesn't have any arguments after it
if (args.length > 1) {
// get the next argument
const start = args[1].getRange().start;
// get the last argument
const end = args[args.length - 1].getRange().end;
// highlight everything after the NONE and warn the user
problems.push(Validator.createHealthcheckNoneUnnecessaryArgument(start, end));
}
// don't need to validate flags of a NONE
break;
} else if (uppercase === "CMD") {
if (args.length === 1) {
// this HEALTHCHECK has a CMD with no arguments
const range = args[0].getRange();
problems.push(Validator.createHealthcheckCmdArgumentMissing(range.start, range.end));
}
} else {
// unknown HEALTHCHECK type
problems.push(Validator.createHealthcheckTypeUnknown(args[0].getRange(), uppercase));
}
}

let validFlags = [ "interval", "retries", "start-period", "timeout" ];
for (let flag of healthcheckFlags) {
let flagName = flag.getName();
if (validFlags.indexOf(flagName) === -1) {
let nameRange = flag.getNameRange();
problems.push(Validator.createUnknownHealthcheckFlag(nameRange.start, nameRange.end, flag.getName()));
} else if (flagName === "retries") {
let value = flag.getValue();
if (value) {
let valueRange = flag.getValueRange();
let integer = parseInt(value);
// test for NaN or numbers with decimals
if (isNaN(integer) || value.indexOf('.') !== -1) {
problems.push(Validator.createInvalidSyntax(valueRange.start, valueRange.end, value));
} else if (integer < 1) {
problems.push(Validator.createFlagAtLeastOne(valueRange.start, valueRange.end, "--retries", integer.toString()));
}
const validFlags = [ "interval", "retries", "start-period", "timeout" ];
for (const flag of healthcheckFlags) {
const flagName = flag.getName();
if (validFlags.indexOf(flagName) === -1) {
const nameRange = flag.getNameRange();
problems.push(Validator.createUnknownHealthcheckFlag(nameRange.start, nameRange.end, flag.getName()));
} else if (flagName === "retries") {
const value = flag.getValue();
if (value) {
const valueRange = flag.getValueRange();
const integer = parseInt(value);
// test for NaN or numbers with decimals
if (isNaN(integer) || value.indexOf('.') !== -1) {
problems.push(Validator.createInvalidSyntax(valueRange.start, valueRange.end, value));
} else if (integer < 1) {
problems.push(Validator.createFlagAtLeastOne(valueRange.start, valueRange.end, "--retries", integer.toString()));
}
}
}

this.checkFlagValue(healthcheckFlags, validFlags, problems);
this.checkFlagDuration(healthcheckFlags, [ "interval", "start-period", "timeout" ], problems);
this.checkDuplicateFlags(healthcheckFlags, validFlags, problems);
}

this.checkFlagValue(healthcheckFlags, validFlags, problems);
this.checkFlagDuration(healthcheckFlags, [ "interval", "start-period", "timeout" ], problems);
this.checkDuplicateFlags(healthcheckFlags, validFlags, problems);
break;
case "ONBUILD":
this.checkArguments(instruction, problems, [ -1 ], function() {
Expand Down Expand Up @@ -875,6 +872,7 @@ export class Validator {
"instructionMissingArgument": "Instruction has no arguments",
"instructionMultiple": "There can only be one ${0} instruction in a Dockerfile",
"instructionRequiresOneArgument": "${0} requires exactly one argument",
"instructionRequiresAtLeastOneArgument": "${0} requires at least one argument",
"instructionRequiresAtLeastTwoArguments": "${0} requires at least two arguments",
"instructionRequiresTwoArguments": "${0} must have two arguments",
"instructionUnnecessaryArgument": "${0} takes no arguments",
Expand Down Expand Up @@ -988,6 +986,10 @@ export class Validator {
return Validator.formatMessage(Validator.dockerProblems["instructionRequiresAtLeastTwoArguments"], "COPY");
}

public static getDiagnosticMessage_HEALTHCHECKRequiresAtLeastOneArgument() {
return Validator.formatMessage(Validator.dockerProblems["instructionRequiresAtLeastOneArgument"], "HEALTHCHECK");
}

public static getDiagnosticMessage_ENVRequiresTwoArguments() {
return Validator.formatMessage(Validator.dockerProblems["instructionRequiresTwoArguments"], "ENV");
}
Expand Down Expand Up @@ -1148,6 +1150,10 @@ export class Validator {
return Validator.createError(start, end, Validator.getDiagnosticMessage_ENVRequiresTwoArguments(), ValidationCode.ARGUMENT_REQUIRES_TWO);
}

private static createHEALTHCHECKRequiresAtLeastOneArgument(range: Range): Diagnostic {
return Validator.createError(range.start, range.end, Validator.getDiagnosticMessage_HEALTHCHECKRequiresAtLeastOneArgument(), ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE);
}

private static createHealthcheckCmdArgumentMissing(start: Position, end: Position): Diagnostic {
return Validator.createError(start, end, Validator.getDiagnosticMessage_HealthcheckCmdArgumentMissing(), ValidationCode.HEALTHCHECK_CMD_ARGUMENT_MISSING);
}
Expand Down
37 changes: 30 additions & 7 deletions test/dockerValidator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,17 @@ function assertENVRequiresTwoArguments(diagnostic: Diagnostic, startLine: number
assert.equal(diagnostic.range.end.character, endCharacter);
}

function assertHEALTHCHECKRequiresAtLeastOneArgument(diagnostic: Diagnostic, startLine: number, startCharacter: number, endLine: number, endCharacter: number) {
assert.equal(diagnostic.code, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE);
assert.equal(diagnostic.severity, DiagnosticSeverity.Error);
assert.equal(diagnostic.source, source);
assert.equal(diagnostic.message, Validator.getDiagnosticMessage_HEALTHCHECKRequiresAtLeastOneArgument());
assert.equal(diagnostic.range.start.line, startLine);
assert.equal(diagnostic.range.start.character, startCharacter);
assert.equal(diagnostic.range.end.line, endLine);
assert.equal(diagnostic.range.end.character, endCharacter);
}

function assertInstructionRequiresOneOrThreeArguments(diagnostic: Diagnostic, startLine: number, startCharacter: number, endLine: number, endCharacter: number) {
assert.equal(diagnostic.code, ValidationCode.ARGUMENT_REQUIRES_ONE_OR_THREE);
assert.equal(diagnostic.severity, DiagnosticSeverity.Error);
Expand Down Expand Up @@ -932,7 +943,7 @@ describe("Docker Validator Tests", function() {
});

it("HEALTHCHECK", function() {
return testMissingArgumentLoop("HEALTHCHECK");
return testMissingArgumentLoop("HEALTHCHECK", false, assertHEALTHCHECKRequiresAtLeastOneArgument);
});

it("LABEL", function() {
Expand Down Expand Up @@ -2299,18 +2310,30 @@ describe("Docker Validator Tests", function() {
it("wrong name", function() {
// no equals sign
let diagnostics = validate("FROM alpine\nHEALTHCHECK --interva");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 14, 1, 21);
assert.equal(diagnostics.length, 2);
assertDiagnostics(
diagnostics,
[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE ],
[ assertUnknownHealthcheckFlag, assertHEALTHCHECKRequiresAtLeastOneArgument ],
[ [ "interva", 1, 14, 1, 21 ], [ 1, 0, 1, 11 ]]);

// empty value
diagnostics = validate("FROM alpine\nHEALTHCHECK --interva=");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 14, 1, 21);
assert.equal(diagnostics.length, 2);
assertDiagnostics(
diagnostics,
[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE ],
[ assertUnknownHealthcheckFlag, assertHEALTHCHECKRequiresAtLeastOneArgument ],
[ [ "interva", 1, 14, 1, 21 ], [ 1, 0, 1, 11 ]]);

// value specified
diagnostics = validate("FROM alpine\nHEALTHCHECK --interva=30s");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 14, 1, 21);
assert.equal(diagnostics.length, 2);
assertDiagnostics(
diagnostics,
[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE ],
[ assertUnknownHealthcheckFlag, assertHEALTHCHECKRequiresAtLeastOneArgument ],
[ [ "interva", 1, 14, 1, 21 ], [ 1, 0, 1, 11 ]]);
});
});
});
Expand Down

0 comments on commit 3fbcf38

Please sign in to comment.