From 532604eb8be8a81b6aa9ae09c6627e62eaa3ff03 Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Mon, 5 Aug 2024 02:31:24 -0400 Subject: [PATCH 1/7] Add option to disallow exemptions, add failing test for compact exemptions --- .../30-disallow-exemptions/bad.model.lkml | 5 ++ .../30-disallow-exemptions/good.model.lkml | 2 + .../30-disallow-exemptions/index.test.js | 67 +++++++++++++++++++ .../30-disallow-exemptions/manifest.lams-lkml | 11 +++ .../31-compact-exemptions/exempt.model.lkml | 10 +++ .../31-compact-exemptions/index.test.js | 36 ++++++++++ .../31-compact-exemptions/manifest.lams-lkml | 5 ++ docs/customizing-lams.md | 6 +- lib/custom-rule/result-interpreter.js | 2 +- lib/get-exemption-deep.js | 13 ++-- 10 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 __tests__/dummy-projects/30-disallow-exemptions/bad.model.lkml create mode 100644 __tests__/dummy-projects/30-disallow-exemptions/good.model.lkml create mode 100644 __tests__/dummy-projects/30-disallow-exemptions/index.test.js create mode 100644 __tests__/dummy-projects/30-disallow-exemptions/manifest.lams-lkml create mode 100644 __tests__/dummy-projects/31-compact-exemptions/exempt.model.lkml create mode 100644 __tests__/dummy-projects/31-compact-exemptions/index.test.js create mode 100644 __tests__/dummy-projects/31-compact-exemptions/manifest.lams-lkml diff --git a/__tests__/dummy-projects/30-disallow-exemptions/bad.model.lkml b/__tests__/dummy-projects/30-disallow-exemptions/bad.model.lkml new file mode 100644 index 0000000..edec5b6 --- /dev/null +++ b/__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???" +# } diff --git a/__tests__/dummy-projects/30-disallow-exemptions/good.model.lkml b/__tests__/dummy-projects/30-disallow-exemptions/good.model.lkml new file mode 100644 index 0000000..1c0d0a8 --- /dev/null +++ b/__tests__/dummy-projects/30-disallow-exemptions/good.model.lkml @@ -0,0 +1,2 @@ + +persist_for: "1 hour" diff --git a/__tests__/dummy-projects/30-disallow-exemptions/index.test.js b/__tests__/dummy-projects/30-disallow-exemptions/index.test.js new file mode 100644 index 0000000..4a3e784 --- /dev/null +++ b/__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" + }); + }); + }); +}); diff --git a/__tests__/dummy-projects/30-disallow-exemptions/manifest.lams-lkml b/__tests__/dummy-projects/30-disallow-exemptions/manifest.lams-lkml new file mode 100644 index 0000000..2155f3a --- /dev/null +++ b/__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 +} diff --git a/__tests__/dummy-projects/31-compact-exemptions/exempt.model.lkml b/__tests__/dummy-projects/31-compact-exemptions/exempt.model.lkml new file mode 100644 index 0000000..8f4755b --- /dev/null +++ b/__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"} +} diff --git a/__tests__/dummy-projects/31-compact-exemptions/index.test.js b/__tests__/dummy-projects/31-compact-exemptions/index.test.js new file mode 100644 index 0000000..61b41d6 --- /dev/null +++ b/__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" + }); + }); + }); +}); diff --git a/__tests__/dummy-projects/31-compact-exemptions/manifest.lams-lkml b/__tests__/dummy-projects/31-compact-exemptions/manifest.lams-lkml new file mode 100644 index 0000000..c298154 --- /dev/null +++ b/__tests__/dummy-projects/31-compact-exemptions/manifest.lams-lkml @@ -0,0 +1,5 @@ + +rule: explore_description { + match: "$.model.*.explore.*" + expr_rule: (!== ::match:description undefined) ;; +} diff --git a/docs/customizing-lams.md b/docs/customizing-lams.md index f44cf4d..5548b7a 100644 --- a/docs/customizing-lams.md +++ b/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: diff --git a/lib/custom-rule/result-interpreter.js b/lib/custom-rule/result-interpreter.js index 62f34a4..23560f0 100644 --- a/lib/custom-rule/result-interpreter.js +++ b/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){ diff --git a/lib/get-exemption-deep.js b/lib/get-exemption-deep.js index 7ab666a..a9653d6 100644 --- a/lib/get-exemption-deep.js +++ b/lib/get-exemption-deep.js @@ -4,11 +4,15 @@ 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 +21,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 +36,6 @@ module.exports = function getExemptionDeep({match, rule}) { ).exempt; if (localExempt) { - return { - match, - exempt: localExempt, - }; + return `Local exemption: ${localExempt}`; } }; From 3ef829170a118312a4e2192c7bf5ce999251a7ca Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Mon, 5 Aug 2024 02:36:11 -0400 Subject: [PATCH 2/7] Implement compact exemptions --- lib/get-exemption.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/get-exemption.js b/lib/get-exemption.js index 5c7eef3..b5a3419 100644 --- a/lib/get-exemption.js +++ b/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; }; From e685a14ae9883c2601b96bcccb8e8eaf09bb1659 Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Mon, 5 Aug 2024 02:36:34 -0400 Subject: [PATCH 3/7] lint --- lib/get-exemption-deep.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/get-exemption-deep.js b/lib/get-exemption-deep.js index a9653d6..7c45716 100644 --- a/lib/get-exemption-deep.js +++ b/lib/get-exemption-deep.js @@ -4,10 +4,11 @@ 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] + const ruleDef = project.manifest && project.manifest.rule && project.manifest.rule[rule]; - if(ruleDef && ruleDef.allow_exemptions === false){ - return} + if (ruleDef && ruleDef.allow_exemptions === false) { + return; + } // Check the manifest for global exemptions const manifestExempt = getExemption(project && project.manifest, rule); From 901a83d251f0db5e212486cace86bd41a60d95ee Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Mon, 5 Aug 2024 02:40:27 -0400 Subject: [PATCH 4/7] Update parser dep --- npm-shrinkwrap.dev.json | 14 +++++++------- npm-shrinkwrap.json | 14 +++++++------- package.json | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/npm-shrinkwrap.dev.json b/npm-shrinkwrap.dev.json index 9a38ba3..16d61be 100644 --- a/npm-shrinkwrap.dev.json +++ b/npm-shrinkwrap.dev.json @@ -13,7 +13,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" }, @@ -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", diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 9a38ba3..16d61be 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -13,7 +13,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" }, @@ -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", diff --git a/package.json b/package.json index 072e108..ccf26a2 100644 --- a/package.json +++ b/package.json @@ -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" }, From 8840624c9d7b090a860659d2f119c5c9bd0056bd Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Mon, 5 Aug 2024 02:58:20 -0400 Subject: [PATCH 5/7] Doc new exemption features --- README.md | 10 ++++------ docs/release-notes/v3.md | 5 +++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 939b15b..0d1f886 100644 --- a/README.md +++ b/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 {...} ... ``` diff --git a/docs/release-notes/v3.md b/docs/release-notes/v3.md index 29e7ebe..1a1e6be 100644 --- a/docs/release-notes/v3.md +++ b/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 From 03029bde02adeacda5648e57fded54963de88a32 Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Mon, 5 Aug 2024 02:59:29 -0400 Subject: [PATCH 6/7] 3.4.0 --- npm-shrinkwrap.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 16d61be..6e9f5cb 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,12 +1,12 @@ { "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", diff --git a/package.json b/package.json index ccf26a2..9a0180c 100644 --- a/package.json +++ b/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": { From bdc9962af708a2ed79eb99af8614227c41dd3326 Mon Sep 17 00:00:00 2001 From: Fabio Beltramini Date: Mon, 5 Aug 2024 03:00:16 -0400 Subject: [PATCH 7/7] Postpublish shrinkwrap updte --- npm-shrinkwrap.dev.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/npm-shrinkwrap.dev.json b/npm-shrinkwrap.dev.json index 16d61be..6e9f5cb 100644 --- a/npm-shrinkwrap.dev.json +++ b/npm-shrinkwrap.dev.json @@ -1,12 +1,12 @@ { "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",