Merge pull request #191 from looker-open-source/exempt-enhancements

Exempt enhancements

README.md

@@ -58,18 +58,16 @@ In addition to linting against its [style guide](https://looker-open-source.gith
 
 ### Rule Exemptions
 
-You can opt-out of rules granularly by locally specifying `rule_exemptions`.
+You can opt-out of rules granularly by locally specifying rule exemptions.
 
-The rule exemption syntax encourages developers to document the reason for each such exemption:
+The rule exemption syntax encourages developers (optionally) to document the reason for each such exemption:
 
 ```lkml
 view: rollup {
   sql_table_name: my_table ;;
 
-  #LAMS
-  #rule_exemptions: {
-  #  K3: "2018-11-12 - Bob said it's ok"
-  #}
+  #LAMS exempt: K3 {why: "Bob said it's ok"}
+
   dimension: info {...}
   ...
 ```

__tests__/dummy-projects/30-disallow-exemptions/bad.model.lkml

@@ -0,0 +1,5 @@
+# LAMS
+# rule_exemptions:{
+#	require_persist_for: "Who needs persist_for???"
+#	really_require_persist_for: "Who really needs persist_for???"
+# }

__tests__/dummy-projects/30-disallow-exemptions/good.model.lkml

@@ -0,0 +1,2 @@
+
+persist_for: "1 hour"

__tests__/dummy-projects/30-disallow-exemptions/index.test.js

@@ -0,0 +1,67 @@
+let {testName, lams, options, mocks} = require('../../../lib/test-commons.js')(__dirname,{dirnameOffset:-1})
+
+options = {...options, manifest:`./manifest.lams-lkml`}
+
+describe('Projects', () => {
+	describe(testName, () => {
+		let {spies, process, console} = mocks()
+		let messages
+		beforeAll( async () => {
+			messages = await lams(options,{process, console});
+		})
+		it("should not error out", ()=> {
+			expect(console.error).not.toHaveBeenCalled()
+		});
+		it("it should not contain any unexpected parser (P0) errors", ()=> {
+			expect({messages}).not.toContainMessage({
+				rule: "P0",
+				level: "error"
+			});
+		});
+		it("it should not contain any parser syntax (P1) errors", ()=> {
+			expect({messages}).not.toContainMessage({
+				rule: "P1",
+				level: "error"
+			});
+		});
+
+		it("it should provide correct aggregate info for require_persist_for (2 match, 1 exempt, 0 error)", ()=> {
+			expect({messages}).toContainMessage({
+				rule: "require_persist_for",
+				level: "info",
+				description: "Rule require_persist_for summary: 2 matches, 1 matches exempt, and 0 errors"
+			});
+		});
+
+		it("it should not error on require_persist_for", ()=> {
+			expect({messages}).not.toContainMessage({
+				rule: "require_persist_for",
+				level: "error"
+			});
+		});
+
+		it("it should provide correct aggregate info for really_require_persist_for (2 match, 0 exempt, 1 error)", ()=> {
+			expect({messages}).toContainMessage({
+				rule: "really_require_persist_for",
+				level: "info",
+				description: "Rule really_require_persist_for summary: 2 matches, 0 matches exempt, and 1 errors"
+			});
+		});
+
+		it("it should error on really_require_persist_for for model bad", ()=> {
+			expect({messages}).toContainMessage({
+				rule: "really_require_persist_for",
+				location: "model:bad",
+				level: "error"
+			});
+		});
+
+		it("it should not error on really_require_persist_for for model good", ()=> {
+			expect({messages}).not.toContainMessage({
+				rule: "really_require_persist_for",
+				location: "model:good",
+				level: "error"
+			});
+		});
+	});
+});

__tests__/dummy-projects/30-disallow-exemptions/manifest.lams-lkml

@@ -0,0 +1,11 @@
+
+rule: require_persist_for {
+	match: "$.model.*"
+	expr_rule: (!== ::match:persist_for undefined) ;;
+}
+
+rule: really_require_persist_for {
+	match: "$.model.*"
+	expr_rule: (!== ::match:persist_for undefined) ;;
+	allow_exemptions: no
+}

__tests__/dummy-projects/31-compact-exemptions/exempt.model.lkml

@@ -0,0 +1,10 @@
+
+explore: bad {}
+
+explore: ex1 {
+	#LAMS exempt: explore_description {}
+}
+
+explore: ex2 {
+	#LAMS exempt: explore_description {why: "Because"}
+}

__tests__/dummy-projects/31-compact-exemptions/index.test.js

@@ -0,0 +1,36 @@
+let {testName, lams, options, mocks} = require('../../../lib/test-commons.js')(__dirname,{dirnameOffset:-1})
+
+options = {...options, manifest:`./manifest.lams-lkml`}
+
+describe('Projects', () => {
+	describe(testName, () => {
+		let {spies, process, console} = mocks()
+		let messages
+		beforeAll( async () => {
+			messages = await lams(options,{process, console});
+		})
+		it("should not error out", ()=> {
+			expect(console.error).not.toHaveBeenCalled()
+		});
+		it("it should not contain any unexpected parser (P0) errors", ()=> {
+			expect({messages}).not.toContainMessage({
+				rule: "P0",
+				level: "error"
+			});
+		});
+		it("it should not contain any parser syntax (P1) errors", ()=> {
+			expect({messages}).not.toContainMessage({
+				rule: "P1",
+				level: "error"
+			});
+		});
+
+		it("it should provide correct aggregate info for explore_description (3 match, 2 exempt, 1 error)", ()=> {
+			expect({messages}).toContainMessage({
+				rule: "explore_description",
+				level: "info",
+				description: "Rule explore_description summary: 3 matches, 2 matches exempt, and 1 errors"
+			});
+		});
+	});
+});

__tests__/dummy-projects/31-compact-exemptions/manifest.lams-lkml

@@ -0,0 +1,5 @@
+
+rule: explore_description {
+	match: "$.model.*.explore.*"
+	expr_rule: (!== ::match:description undefined) ;;
+}

docs/customizing-lams.md

@@ -28,7 +28,8 @@ Expression-based rules can be declared within your manifest's LookML. By way of
 Alternately, you can maintain them in YAML for nicer syntax:
 
 ```
-# in a YAML file passed to the `manifest` argument (requires installing js-yaml)
+# in a YAML file passed to the `manifest` argument
+# (requires installing js-yaml)
 
 rule:
   prod_connection:
@@ -48,8 +49,9 @@ You can quickly experiment with custom rule definitions in the [Rule Sandbox](to
 Here is what each part of the rule definition means:
 
 - **Rule name:** Any LookML name for this rule. This name is included in LAMS usage reporting, if you have opted in to it. Names composed of a single letter followed by a number are reserved for future LAMS usage.
-- **enabled:** (Optional) A yesno value that indicates whether to use the rule. (Default: yes)
+- **allow_exemptions:** (Optional) A yesno value that indicates whether to respect rule exemption declarations. (Default: yes)
 - **description:** (Optional) A succint human-readable description, which will be shown to developers in LAMS' output
+- **enabled:** (Optional) A yesno value that indicates whether to use the rule. (Default: yes)
 - **match:** A [JSONpath expression](https://www.npmjs.com/package/jsonpath-plus) that describes which LookML constructs to check. This usually matches multiple times within the project, and the rule is checked once for each such match. See [Rule Matching](#rule-matching) below for more details and example match patterns.
 - **expr_rule:** A [Liyad](https://github.com/shellyln/liyad) expression that defines the logic of the rule.
   - **Arguments:** Three arguments are made available to the expression:

docs/release-notes/v3.md

@@ -90,3 +90,8 @@ Patches:
 
 	- Enhance the manifest argument to accept paths to lkml, json, or yaml (with js-yaml)
 	- Add manifest-defaults argument
+
+# v3.4
+
+	- Add rule `allow_exemptions` argument for rules to specify if they do not want to allow exemptions
+	- Add support for more compact exemption syntax. The older `rule_exemptions: {rule: "reason"}` syntax is still supported, though no longer emphasized

lib/custom-rule/result-interpreter.js

@@ -29,7 +29,7 @@ module.exports =  function resultInterpreter(ruleDef={}){
 				...resultDefault, 
 				exempt: true,
 				level: "verbose",
-				description: `Exempt: ${result.exempt}`
+				description: `Exempt result: ${exempt}`
 			}]
 		}
 		if(value === undefined){

lib/get-exemption-deep.js

@@ -4,11 +4,16 @@ const formatLocation = require('./custom-rule/format-location.js');
 module.exports = function getExemptionDeep({match, rule}) {
 	// Check match for being exempt and return early (without evaluating expression) if so
 	const {path, project} = match;
+	const ruleDef = project.manifest && project.manifest.rule && project.manifest.rule[rule];
+
+	if (ruleDef && ruleDef.allow_exemptions === false) {
+		return;
+	}
 
 	// Check the manifest for global exemptions
 	const manifestExempt = getExemption(project && project.manifest, rule);
 	if (manifestExempt) {
-		return {match, exempt: manifestExempt};
+		return `Manifest exemption: ${manifestExempt}`;
 	}
 
 	// Check for central exemptions
@@ -17,7 +22,7 @@ module.exports = function getExemptionDeep({match, rule}) {
 			const location = formatLocation(path.slice(0, p+1));
 			const isExempt = project.centralExemptions.has(rule + ' ' + location);
 			if (isExempt) {
-				return {match, exempt: `Central exemption for ${location}`};
+				return `Central exemption: ${location}`;
 			}
 		}
 	}
@@ -32,9 +37,6 @@ module.exports = function getExemptionDeep({match, rule}) {
 	).exempt;
 
 	if (localExempt) {
-		return {
-			match,
-			exempt: localExempt,
-		};
+		return `Local exemption: ${localExempt}`;
 	}
 };

lib/get-exemption.js

@@ -5,5 +5,7 @@
  * @return {String|boolean} Return reason if rule is exempt otherwise false
  */
 exports = module.exports = function getExemption(obj, rule) {
-	return (obj && obj.rule_exemptions && typeof obj.rule_exemptions[rule] === 'string' && obj.rule_exemptions[rule]) || false;
+	return (obj && obj.rule_exemptions && typeof obj.rule_exemptions[rule] === 'string' && obj.rule_exemptions[rule])
+		|| (obj && obj.exempt && typeof obj.exempt[rule] === 'object')
+		|| false;
 };

npm-shrinkwrap.dev.json

@@ -1,19 +1,19 @@
 {
   "name": "@looker/look-at-me-sideways",
-  "version": "3.3.0",
+  "version": "3.4.0",
   "lockfileVersion": 2,
   "requires": true,
   "packages": {
     "": {
       "name": "@looker/look-at-me-sideways",
-      "version": "3.3.0",
+      "version": "3.4.0",
       "license": "MIT",
       "dependencies": {
         "dot": "^1.1.3",
         "fromentries": "^1.3.2",
         "jsonpath-plus": "^7.2.0",
         "liyad": "^0.2.4",
-        "lookml-parser": "^6.9.1",
+        "lookml-parser": "^6.10.0",
         "minimist": "^1.2.6",
         "require-from-string": "^2.0.2"
       },
@@ -3809,9 +3809,9 @@
       "dev": true
     },
     "node_modules/lookml-parser": {
-      "version": "6.9.1",
-      "resolved": "https://registry.npmjs.org/lookml-parser/-/lookml-parser-6.9.1.tgz",
-      "integrity": "sha512-2xq76z1Igpnz1/j0o14qxHp6FpWfDRvmDVKjk71RouLW81yTp6NAqwyCM5r0UdjIgc9RTPGNY6G2DwG1XBWqFg==",
+      "version": "6.10.0",
+      "resolved": "https://registry.npmjs.org/lookml-parser/-/lookml-parser-6.10.0.tgz",
+      "integrity": "sha512-eA8+zt7wUKXnkx+7M86Z7mRqta3+TZhMkYTVmyMau+F7a5SzjMhsXzQqWmQgyZmD/iRElWyb+6LQ2b/5SSwBaQ==",
       "dependencies": {
         "bluebird": "^3.5.1",
         "glob": "^7.1.2",
@@ -7970,9 +7970,9 @@
       "dev": true
     },
     "lookml-parser": {
-      "version": "6.9.1",
-      "resolved": "https://registry.npmjs.org/lookml-parser/-/lookml-parser-6.9.1.tgz",
-      "integrity": "sha512-2xq76z1Igpnz1/j0o14qxHp6FpWfDRvmDVKjk71RouLW81yTp6NAqwyCM5r0UdjIgc9RTPGNY6G2DwG1XBWqFg==",
+      "version": "6.10.0",
+      "resolved": "https://registry.npmjs.org/lookml-parser/-/lookml-parser-6.10.0.tgz",
+      "integrity": "sha512-eA8+zt7wUKXnkx+7M86Z7mRqta3+TZhMkYTVmyMau+F7a5SzjMhsXzQqWmQgyZmD/iRElWyb+6LQ2b/5SSwBaQ==",
       "requires": {
         "bluebird": "^3.5.1",
         "glob": "^7.1.2",

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@looker/look-at-me-sideways",
-  "version": "3.3.0",
+  "version": "3.4.0",
   "description": "A judgmental little LookML linter >_>",
   "main": "index.js",
   "directories": {
@@ -39,7 +39,7 @@
     "fromentries": "^1.3.2",
     "jsonpath-plus": "^7.2.0",
     "liyad": "^0.2.4",
-    "lookml-parser": "^6.9.1",
+    "lookml-parser": "^6.10.0",
     "minimist": "^1.2.6",
     "require-from-string": "^2.0.2"
   },