feat: allow spectral rules to be defined in the validaterc #202

rmkeezer · 2020-10-02T15:02:10Z

ref: https://github.ibm.com/arf/planning-sdk-squad/issues/2170
followup of: #195

These changes allow spectral rules to have their severity level changed in the .validatrc and allow custom rules defined in a .spectral.yaml file to not be overridden by the .defaultsForSpectral.yaml. Although the .defaultsForSpectral.yaml will still override the .spectral.yaml severity level of any existing rule .defaultsForSpectral.yaml defines a severity for, including the extended rules.

src/cli-validator/utils/processConfiguration.js

jorge-ibm · 2020-10-02T17:25:22Z

src/spectral/rulesets/.defaultsForSpectral.yaml

@@ -1,6 +1,6 @@
 extends: [[spectral:oas, off]]
 formats: [oas2, oas3]
-functionsDir": ../functions
+functionsDir: ../functions


Surprised spectral didn't complain about that

jorge-ibm · 2020-10-02T17:30:26Z

src/spectral/utils/spectral-validator.js

+  // Combine user ruleset with the default ruleset
+  // The default ruleset will take precendence in any rules it defines (including extended rules)
+  const defaultRuleset = await readRuleset(defaultSpectralRulesetURI);
+  mergeRules(spectralRuleset.rules, defaultRuleset.rules);


Might be worth trying to see if you can achieve the same merging by simply calling await spectral.loadRuleset([spectralRuleset, defaultSpectralRulesetURI]);

Yup this does achieve the same, I wasn't confident it did at first but it seems to be working as expect! I updated the PR with this

jorge-ibm

It's looking better, just a couple of more things that need to be changed. Also, if possible, some unit tests to test the validaterc changes would be helpful.

jorge-ibm · 2020-10-05T14:41:58Z

src/.defaultsForValidator.js

@@ -106,6 +106,30 @@ const defaults = {
    'schemas': {
      'json_or_param_binary_string': 'warning'
    }
+  },
+  'spectral': {


Question: what'll happen if this new section is missing from the validaterc? Just want to ensure that not having this new section will not break current users.

We also need to update the init command to generate the new section we're adding here.

Since i'm calling getConfigObject if there is no spectral section in the validaterc, it will take the rules from the spectral rules from .defaultsForValidator.js and these changes also allow the init command to generate the new spectral section

jorge-ibm · 2020-10-05T15:52:37Z

src/spectral/utils/spectral-validator.js

  );

+  // Combine user ruleset with the default ruleset
+  // The default ruleset will take precendence in any rules it defines (including extended rules)
+  await spectral.loadRuleset([spectralRulesetURI, defaultSpectralRulesetURI]);


Now that we're using loadRuleset we should update this to load a custom user config AND our default ruleset iff a user provided a custom ruleset file. The way you have it now, spectralRulesetURI would just return the default ruleset if a user didn't specify a custom config so you would load the same ruleset twice.

Also, I think we should load our ruleset file first, that way we don't unintentionally override any openapi rules they might pull in their ruleset file.

I dont think there is a problem with loading a ruleset twice since it won't create any duplicates. Currently it will load the user config and the .defaultsForSpectral.yaml config or the .defaultsForSpectral.yaml config twice, how should this be changed? I can change it to load the .defaultsForSpectral.yaml first, should I make any other changes?

Okay. Yes, I think we should load the .defaultsForSpectral.yaml first

Ok i updated it to load the defaults first

jorge-ibm · 2020-10-06T15:50:23Z

@rmkeezer I'm not sure if my suggestion to use loadRuleset vs mergeRules broke something, but I pulled your fork locally and wasn't able to overwrite the severity level of our rules using the validaterc file.

rmkeezer · 2020-10-06T18:39:30Z

@jorge-ibm Ah, apparently spectral.mergeRules does not seem to work at all, but using the mergeRules function from the spectral ruleset does. I've updated the PR with that change, it should now correctly override the severity again.

jorge-ibm · 2020-10-06T21:51:31Z

Looks like everything is working as expected now, though I still think some tests would be helpful. The only thing I think should be fixed before merging is this duplicate message displaying whenever the validaterc file isn't present:

JorgeWoComputer:openapi-validator-rkeezer jrangel$ lint-openapi -p test/spectral/mockFiles/oas3/enabled-rules.yml 
[Warning] No .validaterc file found. The validator will run in default mode.
To configure the validator, create a .validaterc file.
[Warning] No .validaterc file found. The validator will run in default mode.
To configure the validator, create a .validaterc file.

I'm assuming we want to add documentation on how to enable/edit our spectral rules from the validaterc file. @dpopp07 what are your thoughts?

jorge-ibm

Sorry for requesting another set of changes, but I think this PR is really close.

jorge-ibm

LGTM. I'll approve but we should hold off on merging until @dpopp07 or @mkistler review it as well

dpopp07

This looks good! I left a few minor comments but the primary reason I am "requesting changes" is because we need to have some tests for this behavior to ensure it does what we expect it to and to prevent regressions as we keep working on the Spectral stuff

src/cli-validator/utils/processConfiguration.js

dpopp07 · 2020-10-07T20:16:42Z

src/spectral/utils/spectral-validator.js

@@ -46,7 +48,7 @@ const parseResults = function(results, debug) {
 };

 // setup: registers the oas2/oas3 formats, and attempts to load the ruleset file
-const setup = async function(spectral) {
+const setup = async function(spectral, defaultMode = false) {


Is there anywhere setup would be called without specifying this value? It looks like the one place we call it, we provide the value. So this default seems redundant. If it can be called in other places, that's a different story!

Okay, I found it 🙂 This happens on this line but we have the default value there as a variable - so I think it would be better to pass it in that let it be potentially redefined. This should be addressed before approval

Thanks for making that change - will you remove the default value here now?

Suggested change

const setup = async function(spectral, defaultMode = false) {

const setup = async function(spectral, defaultMode) {

dpopp07 · 2020-10-07T20:20:40Z

src/spectral/utils/spectral-validator.js

  );

+  // Combine user ruleset with the default ruleset
+  // The default ruleset will take precendence in any rules it defines (including extended rules)
+  await spectral.loadRuleset([defaultSpectralRulesetURI, spectralRulesetURI]);


So this handles the case in which users define their own configuration with the Spectral format? But we don't let them override our default Spectral rules with that format?

I think this is what we want but as someone not very familiar with Spectral, I find it a little confusing. I appreciate the comments here, they are helpful, but will you add a couple more to provide context for why this is there?

Matthew correct me if I'm wrong but with this PR this is what the user can do now:

They can override the severity level and disable/enable our spectral rules from the validaterc file.

They can add any new custom rules to the validator using a custom rule set. Even if they don't include our spectral rules in their ruleset file, our spectral rules will still be loaded.

If a user wants to override our spectral rules using their ruleset file, they would have to:

remove that spectral rule from the validaterc file

add a rule with the same name to their ruleset file

Correct

Correct

Correct, but I realized that the omitting the spectral rule from the validaterc file did not remove it and still overrode the severity the user would specify.

This was due to the config.get() function getting the defaultObject when there was no validaterc found, this default object specified the same rules as the default validaterc and would take priority. So I added a new flag to prevent this defaultObject from overriding it so it acts as you should expect it to like 3.

This sounds good although I'm slightly confused because if there is no validaterc file, the default object would be used, and the default object doesn't include the spectral rules. So it should be the same no spectral rules being defined, right?

rmkeezer · 2020-10-12T17:25:37Z

package.json

@@ -8,7 +8,8 @@
  "scripts": {
    "link": "npm install -g",
    "unlink": "npm uninstall -g",
-    "test": "jest test/",
+    "test": "jest test/ && jest test/spectral/tests/spectral-validator.test.js --runInBand",
+    "test:seq": "jest test/spectral/tests/spectral-validator.test.js --runInBand",


I added tests to check reading the default config .validaterc, and the user config .spectral.yaml, but to test them I had the test create a temporary config file.

However, this interferes with other tests when run in parallel, so I have this test file running sequentially after the regular tests.

Is there a better solution for tests like these or is it fine to run this file sequentially?

Maybe try to spy on the fs package? https://stackoverflow.com/questions/50066138/mock-fs-function-with-jest . Not sure, @dpopp07 might have more experience doing something like that.

We should be able to write tests that don't require this step but I don't fully understand the issue yet so I'll need to look at it a little more

dpopp07

Getting better but I think there are some simplifications to be made that I did not notice before, specifically around reading the config object. Let me know what you think

dpopp07 · 2020-10-13T19:36:05Z

src/spectral/utils/spectral-validator.js

@@ -46,7 +48,7 @@ const parseResults = function(results, debug) {
 };

 // setup: registers the oas2/oas3 formats, and attempts to load the ruleset file
-const setup = async function(spectral) {
+const setup = async function(spectral, defaultMode = false) {


Thanks for making that change - will you remove the default value here now?

Suggested change

const setup = async function(spectral, defaultMode = false) {

const setup = async function(spectral, defaultMode) {

dpopp07 · 2020-10-13T19:59:08Z

package.json

@@ -8,7 +8,8 @@
  "scripts": {
    "link": "npm install -g",
    "unlink": "npm uninstall -g",
-    "test": "jest test/",
+    "test": "jest test/ && jest test/spectral/tests/spectral-validator.test.js --runInBand",
+    "test:seq": "jest test/spectral/tests/spectral-validator.test.js --runInBand",


We should be able to write tests that don't require this step but I don't fully understand the issue yet so I'll need to look at it a little more

dpopp07 · 2020-10-13T21:53:26Z

src/spectral/utils/spectral-validator.js

+
+  // Combine default/user ruleset with the validaterc spectral rules
+  // The validaterc rules will take precendence in the case of duplicate rules
+  let configObject = await config.get(defaultMode, chalk, null, true, true);


Okay I think I have a way around these new arguments. The config.get function was never meant to be called more than once, which is why we’re seeing these oddities. We already have the processed configObject, which should include the spectral rules, before this setup method is ever called. We should just pass it in here and use that.

We can even pass in just the spectral rules and default to an empty object. If no spectral rules are passed in, we assume the defaults but let the user override

dpopp07 · 2020-10-13T21:55:51Z

src/cli-validator/runValidator.js

@@ -166,7 +166,7 @@ const processInput = async function(program) {
  // or the default ruleset
  const spectral = new Spectral();
  try {
-    await spectralValidator.setup(spectral);
+    await spectralValidator.setup(spectral, defaultMode);


We should pass the config object directly to the spectral module so that it doesn't need to be read again. We might even be able to pass in something like configObject.spectral to simplify the logic in the module (I'll let you sort that out).

Suggested change

await spectralValidator.setup(spectral, defaultMode);

await spectralValidator.setup(spectral, configObject);

dpopp07 · 2020-10-13T21:57:35Z

src/cli-validator/utils/processConfiguration.js

+  chalk,
+  configFileOverride,
+  silentMode,
+  doNotUseDefault


If we pass the config object to the spectral module in runValidator, we won't need to call this method again and these two new arguments are no longer needed

dpopp07 · 2020-10-13T22:00:50Z

src/spectral/utils/spectral-validator.js

  );

+  // Combine user ruleset with the default ruleset
+  // The default ruleset will take precendence in any rules it defines (including extended rules)
+  await spectral.loadRuleset([defaultSpectralRulesetURI, spectralRulesetURI]);


This sounds good although I'm slightly confused because if there is no validaterc file, the default object would be used, and the default object doesn't include the spectral rules. So it should be the same no spectral rules being defined, right?

dpopp07 · 2020-10-13T22:05:00Z

test/spectral/tests/spectral-validator.test.js

@@ -108,3 +112,155 @@ describe('spectral - test spectral-validator.js', function() {
    );
  });
 });
+
+const sequential = process.argv.includes('--runInBand');


My only question on the tests is - is there a way to just look at what rules spectral is ready to use after it's defined? Then we could just call the setup function and make sure it does what we want it to. I think that would be much simpler.

That said, I do like the idea of having an end-to-end test to verify the behavior we're looking for in a real life scenario. We can discuss later

I was able to spyOn the config files so the end-to-end test isn't as troublesome anymore. For non-end-to-end tests, it's a bit difficult with spectral since creating a ruleset needs a file first and add/modifying rules is a bit messy using spectral's functions.

rmkeezer · 2020-10-14T21:42:15Z

I updated the PR with @dpopp07 's suggestions for using the defaultObject instead of calling config.get() inside the spectral run. This worked well! I also updated the unit test to using jest.spyOn to replace the file URL and the returned config file with their respective spectral changes. So the test does not need to run sequentially anymore!

jorge-ibm

Looks really good! My last recommendation would be to add at least 1 test case that uses the commandLineValidator vs the inCodeValidator to invoke the validator.

jorge-ibm · 2020-10-14T21:50:47Z

test/spectral/tests/spectral-validator.test.js

+    const defaultMode = false;
+    validationResults = await inCodeValidator(oas3InMemory, defaultMode);
+
+    // Ensure mockConig was called and revert it to its original state


Suggested change

// Ensure mockConig was called and revert it to its original state

// Ensure mockConfig was called and revert it to its original state

jorge-ibm · 2020-10-14T21:57:20Z

test/spectral/tests/spectral-validator.test.js

+  // There should be no errors and 2 warnings for a rule not off
+  it('test no errors and no warnings', function() {
+    expect(validationResults.errors.length).toBe(0);
+    expect(validationResults.warnings.length).toBe(2);


Are the two warnings from a non-spectral rule?

Yup, there is a warning for Definition was declared but never used in document, i'm clarifying the comment for this

jorge-ibm · 2020-10-14T21:58:48Z

src/spectral/utils/spectral-validator.js

+  await spectral.loadRuleset([defaultSpectralRulesetURI, spectralRulesetURI]);
+
+  // Combine default/user ruleset with the validaterc spectral rules
+  // The validaterc rules will take precendence in the case of duplicate ruless


Suggested change

// The validaterc rules will take precendence in the case of duplicate ruless

// The validaterc rules will take precendence in the case of duplicate rules

dpopp07

Looks good! I left some extremely minor comments to address before merging but I think it's good otherwise

dpopp07 · 2020-10-14T22:03:16Z

src/spectral/utils/spectral-validator.js

+  await spectral.loadRuleset([defaultSpectralRulesetURI, spectralRulesetURI]);
+
+  // Combine default/user ruleset with the validaterc spectral rules
+  // The validaterc rules will take precendence in the case of duplicate ruless


Suggested change

// The validaterc rules will take precendence in the case of duplicate ruless

// The validaterc rules will take precendence in the case of duplicate rules

dpopp07 · 2020-10-14T22:05:46Z

src/cli-validator/utils/processConfiguration.js

@@ -10,6 +10,8 @@ const defaultConfig = require('../../.defaultsForValidator');
 // global objects
 const readFile = util.promisify(fs.readFile);
 const defaultObject = defaultConfig.defaults;
+// Clear spectral rules from defaultObject


I think this makes sense but I was confused at first - maybe just add a bit more info in the comment like

Suggested change

// Clear spectral rules from defaultObject

// Clear spectral rules from defaultObject, only use the explicit values in the validaterc file

// the default values for spectral come from another file

mkistler

Looks good! 👍

# [0.31.0](v0.30.1...v0.31.0) (2020-10-15) ### Features * allow spectral rule severity to be modified in the validaterc ([#202](#202)) ([dc81079](dc81079))

dpopp07 · 2020-10-15T15:57:44Z

🎉 This PR is included in version 0.31.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rmkeezer requested review from dpopp07 and jorge-ibm October 2, 2020 15:02

rmkeezer force-pushed the spectral_config_refactor branch 2 times, most recently from fb0b6d6 to 52e53ff Compare October 2, 2020 16:53

rmkeezer commented Oct 2, 2020

View reviewed changes

src/cli-validator/utils/processConfiguration.js Outdated Show resolved Hide resolved

jorge-ibm reviewed Oct 2, 2020

View reviewed changes

rmkeezer force-pushed the spectral_config_refactor branch from 52e53ff to fea6b48 Compare October 5, 2020 13:32

rmkeezer mentioned this pull request Oct 5, 2020

When running with Spectral rule not all non spectral warnings are returned #200

Closed

rmkeezer force-pushed the spectral_config_refactor branch from fea6b48 to 47f5156 Compare October 5, 2020 16:08

jorge-ibm suggested changes Oct 5, 2020

View reviewed changes

rmkeezer force-pushed the spectral_config_refactor branch from 47f5156 to 0f8b165 Compare October 6, 2020 14:37

rmkeezer force-pushed the spectral_config_refactor branch 2 times, most recently from ae2d029 to 08722d2 Compare October 6, 2020 18:37

rmkeezer force-pushed the spectral_config_refactor branch from 08722d2 to 5e72858 Compare October 6, 2020 18:41

rmkeezer requested a review from jorge-ibm October 6, 2020 18:42

jorge-ibm suggested changes Oct 6, 2020

View reviewed changes

rmkeezer force-pushed the spectral_config_refactor branch from 5e72858 to f9f1354 Compare October 7, 2020 15:55

rmkeezer requested a review from jorge-ibm October 7, 2020 19:13

jorge-ibm approved these changes Oct 7, 2020

View reviewed changes

jorge-ibm requested a review from mkistler October 7, 2020 19:39

dpopp07 requested changes Oct 7, 2020

View reviewed changes

rmkeezer force-pushed the spectral_config_refactor branch 2 times, most recently from 4f22704 to 0b126a7 Compare October 12, 2020 15:46

rmkeezer commented Oct 12, 2020

View reviewed changes

rmkeezer requested a review from dpopp07 October 12, 2020 17:25

dpopp07 requested changes Oct 13, 2020

View reviewed changes

rmkeezer force-pushed the spectral_config_refactor branch from 0b126a7 to 3183944 Compare October 14, 2020 21:33

rmkeezer force-pushed the spectral_config_refactor branch from 3183944 to d722281 Compare October 14, 2020 21:39

rmkeezer requested review from dpopp07 and jorge-ibm October 14, 2020 21:40

jorge-ibm reviewed Oct 14, 2020

View reviewed changes

dpopp07 approved these changes Oct 14, 2020

View reviewed changes

jorge-ibm approved these changes Oct 14, 2020

View reviewed changes

rmkeezer force-pushed the spectral_config_refactor branch 2 times, most recently from a3cc44d to 5d99f80 Compare October 15, 2020 15:07

feat: allow spectral rule severity to be modified in the validaterc

6b2eb7b

rmkeezer force-pushed the spectral_config_refactor branch from 5d99f80 to 6b2eb7b Compare October 15, 2020 15:08

mkistler approved these changes Oct 15, 2020

View reviewed changes

rmkeezer merged commit dc81079 into IBM:master Oct 15, 2020

dpopp07 pushed a commit that referenced this pull request Oct 15, 2020

chore(release): 0.31.0 [skip ci]

0c21837

# [0.31.0](v0.30.1...v0.31.0) (2020-10-15) ### Features * allow spectral rule severity to be modified in the validaterc ([#202](#202)) ([dc81079](dc81079))

dpopp07 added the released label Oct 15, 2020

	const setup = async function(spectral, defaultMode = false) {
	const setup = async function(spectral, defaultMode) {

	await spectralValidator.setup(spectral, defaultMode);
	await spectralValidator.setup(spectral, configObject);

	// Ensure mockConig was called and revert it to its original state
	// Ensure mockConfig was called and revert it to its original state

	// The validaterc rules will take precendence in the case of duplicate ruless
	// The validaterc rules will take precendence in the case of duplicate rules

	// Clear spectral rules from defaultObject
	// Clear spectral rules from defaultObject, only use the explicit values in the validaterc file
	// the default values for spectral come from another file

feat: allow spectral rules to be defined in the validaterc #202

feat: allow spectral rules to be defined in the validaterc #202

Conversation

rmkeezer commented Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorge-ibm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorge-ibm commented Oct 6, 2020

rmkeezer commented Oct 6, 2020 • edited Loading

jorge-ibm commented Oct 6, 2020

jorge-ibm left a comment

Choose a reason for hiding this comment

jorge-ibm left a comment

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmkeezer Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmkeezer commented Oct 14, 2020

jorge-ibm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkistler left a comment

Choose a reason for hiding this comment

dpopp07 commented Oct 15, 2020

rmkeezer commented Oct 2, 2020 •

edited

Loading

rmkeezer commented Oct 6, 2020 •

edited

Loading

rmkeezer Oct 12, 2020 •

edited

Loading