Fix #173 Warn if HEALTHCHECK's argument is unknown

Signed-off-by: Remy Suen <[email protected]>
rcjsuen · Sep 14, 2017 · a8186b4 · a8186b4
1 parent 3bca970
commit a8186b4
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,8 @@ All notable changes to this project will be documented in this file.
   - create docker.command.flagToCopyFrom to convert an unknown COPY flag ([#171](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/171))
 - textDocument/completion
   - 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))
 
 ### Fixed
 - correct the documentation of HEALTHCHECK's --retries flag's completion item ([#170](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/170))

diff --git a/src/dockerValidator.ts b/src/dockerValidator.ts
@@ -55,6 +55,7 @@ export enum ValidationCode {
 	UNKNOWN_INSTRUCTION,
 	UNKNOWN_COPY_FLAG,
 	UNKNOWN_HEALTHCHECK_FLAG,
+	UNKNOWN_TYPE,
 	DEPRECATED_MAINTAINER,
 	HEALTHCHECK_CMD_ARGUMENT_MISSING
 }
@@ -274,7 +275,7 @@ export class Validator {
 				}
 			}
 
-			validateInstruction: switch (keyword) {
+			switch (keyword) {
 				case "CMD":
 					// don't validate CMD instructions
 					break;
@@ -357,27 +358,31 @@ export class Validator {
 						let range = instruction.getInstructionRange();
 						problems.push(Validator.createMissingArgument(range.start, range.end));
 					} else {
-						// check all the args
-						for (let i = 0; i < args.length; i++) {
-							let value = args[i].getValue();
+						// check the first argument
+						if (args.length > 0) {
+							let value = args[0].getValue();
 							let uppercase = value.toUpperCase();
 							if (uppercase === "NONE") {
-								if (i + 1 <= args.length - 1) {
+								// check that NONE doesn't have any arguments after it
+								if (args.length > 1) {
 									// get the next argument
-									let start = args[i + 1].getRange().start;
+									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 validateInstruction;
+								break;
 							} else if (uppercase === "CMD") {
-								if (i === args.length - 1) {
-									let range = args[i].getRange();
+								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));
 								}
-								break;
+							} else {
+								// unknown HEALTHCHECK type
+								problems.push(Validator.createHealthcheckTypeUnknown(args[0].getRange(), uppercase));
 							}
 						}
 
@@ -885,6 +890,7 @@ export class Validator {
 		"deprecatedMaintainer": "MAINTAINER has been deprecated",
 
 		"healthcheckCmdArgumentMissing": "Missing command after HEALTHCHECK CMD",
+		"healthcheckTypeUnknown": "Unknown type\"${0}\" in HEALTHCHECK (try CMD)"
 	};
 
 	private static formatMessage(text: string, ...variables: string[]): string {
@@ -1046,6 +1052,10 @@ export class Validator {
 		return Validator.dockerProblems["healthcheckCmdArgumentMissing"];
 	}
 
+	public static getDiagnosticMessage_HealthcheckTypeUnknown(type: string) {
+		return Validator.formatMessage(Validator.dockerProblems["healthcheckTypeUnknown"], type);
+	}
+
 	static createInvalidEscapeDirective(start: Position, end: Position, value: string): Diagnostic {
 		return Validator.createError(start, end, Validator.getDiagnosticMessage_DirectiveEscapeInvalid(value), ValidationCode.INVALID_ESCAPE_DIRECTIVE);
 	}
@@ -1142,6 +1152,10 @@ export class Validator {
 		return Validator.createError(start, end, Validator.getDiagnosticMessage_HealthcheckCmdArgumentMissing(), ValidationCode.HEALTHCHECK_CMD_ARGUMENT_MISSING);
 	}
 
+	private static createHealthcheckTypeUnknown(range: Range, type: string): Diagnostic {
+		return Validator.createError(range.start, range.end, Validator.getDiagnosticMessage_HealthcheckTypeUnknown(type), ValidationCode.UNKNOWN_TYPE);
+	}
+
 	private static createOnbuildChainingDisallowed(range: Range): Diagnostic {
 		return Validator.createError(range.start, range.end, Validator.getDiagnosticMessage_OnbuildChainingDisallowed(), ValidationCode.ONBUILD_CHAINING_DISALLOWED);
 	}

diff --git a/test/dockerValidator.test.ts b/test/dockerValidator.test.ts
@@ -286,6 +286,17 @@ function assertHealthcheckCmdArgumentMissing(diagnostic: Diagnostic, startLine:
 	assert.equal(diagnostic.range.end.character, endCharacter);
 }
 
+function assertHealthcheckTypeUnknown(diagnostic: Diagnostic, type: string, startLine: number, startCharacter: number, endLine: number, endCharacter: number) {
+	assert.equal(diagnostic.code, ValidationCode.UNKNOWN_TYPE);
+	assert.equal(diagnostic.severity, DiagnosticSeverity.Error);
+	assert.equal(diagnostic.source, source);
+	assert.equal(diagnostic.message, Validator.getDiagnosticMessage_HealthcheckTypeUnknown(type));
+	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 assertCOPYRequiresAtLeastTwoArguments(diagnostic: Diagnostic, startLine: number, startCharacter: number, endLine: number, endCharacter: number) {
 	assert.equal(diagnostic.code, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_TWO);
 	assert.equal(diagnostic.severity, DiagnosticSeverity.Error);
@@ -2251,6 +2262,38 @@ describe("Docker Validator Tests", function() {
 			});
 		});
 
+		describe("unknown type", function() {
+			it("argument only", function() {
+				let diagnostics = validate("FROM alpine\nHEALTHCHECK TEST");
+				assert.equal(diagnostics.length, 1);
+				assertHealthcheckTypeUnknown(diagnostics[0], "TEST", 1, 12, 1, 16);
+
+				diagnostics = validate("FROM alpine\nHEALTHCHECK test");
+				assert.equal(diagnostics.length, 1);
+				assertHealthcheckTypeUnknown(diagnostics[0], "TEST", 1, 12, 1, 16);
+			});
+
+			it("flags", function() {
+				let diagnostics = validate("FROM alpine\nHEALTHCHECK --interval=10s TEST");
+				assert.equal(diagnostics.length, 1);
+				assertHealthcheckTypeUnknown(diagnostics[0], "TEST", 1, 27, 1, 31);
+
+				diagnostics = validate("FROM alpine\nHEALTHCHECK --interval=10s test");
+				assert.equal(diagnostics.length, 1);
+				assertHealthcheckTypeUnknown(diagnostics[0], "TEST", 1, 27, 1, 31);
+			});
+
+			it("invalid flags", function() {
+				let diagnostics = validate("FROM alpine\nHEALTHCHECK --intervul=10s TEST");
+				assert.equal(diagnostics.length, 2);
+				assertDiagnostics(
+					diagnostics,
+					[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.UNKNOWN_TYPE ],
+					[ assertUnknownHealthcheckFlag, assertHealthcheckTypeUnknown ],
+					[ [ "intervul", 1, 14, 1, 22 ], [ "TEST", 1, 27, 1, 31 ]]);
+			});
+		});
+
 		describe("unspecified", function() {
 			describe("flags", function() {
 				it("wrong name", function() {