feat: custom rulesets #129

philsturgeon · 2020-02-25T10:25:42Z

Work with custom rulesets in Spectral v5.0.

rubenfiszel · 2020-03-26T18:21:06Z

We love spectral and the action. It is a bit noisy on our side and we would like to disable some checks. It seems that we could only do that through custom rulesets so we're very eager to see progress on this PR. Is it blocked by something hard? I could help if needed.

philsturgeon · 2020-03-26T18:27:23Z

@rubenfiszel hey, I would love to have this feature too. It's been "coming soon" for a while, but it's the very next task that my team works on. This PR will be picked up by a team mate and should be done... well soon. This whole pandemic is messing with everyones predictions of time so no promises, but it is next.

rubenfiszel · 2020-03-26T18:29:42Z

Got it, thanks for the update and the awesome action!

nulltoken · 2020-04-25T05:49:44Z

@philsturgeon Started to work on this

nulltoken · 2020-04-25T19:27:55Z

@philsturgeon I've got something that starts to work...

cf. https://github.com/stoplightio/spectral-action/pull/129/checks

The workflow that runs on every push/pull request now use a custom ruleset with exceptions (which are honored).

@XVincentX @P0lip Would you please take a peek at this proposal?

README.md

nulltoken · 2020-04-25T20:13:29Z

@XVincentX @P0lip The docker build spits an insane amount of warnings:

And I'm not sure how to fix them... :-/

Help? 🙏

nulltoken · 2020-04-25T21:26:29Z

@philsturgeon @XVincentX So I've created a dedicated repo (https://github.com/stoplightio/spectral-action-example/) to test the process out from a consumer standpoint.

https://github.com/stoplightio/spectral-action-example/pull/1/checks?check_run_id=618592904

Looks like I still need to iron out some things...

nulltoken · 2020-04-26T20:10:48Z

@philsturgeon @XVincentX @P0lip I think I'm done.

custom rulesets now work
annotations are now hooked to the proper commit, whether in push or in pull request context
build has been improved (a bit) to avoid some warnings during the docker build phase
some logging has been added to help future potential troubleshooting
configuration from the user standpoint is slightly easier (explicitly passing the GITHUB_TOKEN is no longer needed)
Lint check runs are now suffixed with the event name to avoid having one of them disappear because of name collision:

Some pre-existing issues are still there:

uri.js warnings during the Docker build phase
Lint check runs are randomly attached to any workflow (known GHActionsv2 issues: see Unable to create annotations actions/toolkit#133 (comment) and Access to the job name? actions/toolkit#74 for instance). I've tried a lot to work around that but didn't came up with a reliable outcome)

nulltoken · 2020-04-26T20:24:18Z

@philsturgeon https://github.com/stoplightio/spectral-action-example/pull/1/files is now properly decorated.

However, you'll notice that Annotations seem to be hooked to the last line of the offending code block... The annotation is correctly created (from the GH api standpoint), but the resulting UI is causing me violent mixed feelings.

Thoughts?

nulltoken · 2020-04-27T15:54:37Z

@rubenfiszel If you'd like to get an early peek and participate in the review from a user standpoint, that would be amazing and your feedback would be very welcome!

https://github.com/stoplightio/spectral-action-example/pull/1/files shows how to reference the action from this very branch.

Would you be willing to give it a try, beware that this branch is going to be short lived. So, don't merge any reference to it. It'd be safer to keep the changes in a opened PR on your side until it's reviewed, fixed, merged and officially released.

XVincentX · 2020-04-27T15:58:54Z

I'll try to check out this later today!

rubenfiszel · 2020-04-27T16:51:17Z

@nulltoken Tried on our repo. It works as expected. Thank you very much!

XVincentX · 2020-04-27T20:26:31Z

.vscode/extensions.json

@@ -0,0 +1,5 @@
+{
+  // See https://go.microsoft.com/fwlink/?LinkId=827846


Do we really need this file in the repository?

@XVincentX This will pop up a dialog driving the user to install the editorconfig extension to try and keep the style clean by automatically applying fixups on save (trimming trailing whitespaces, ...). Of course, that will only work with vscode.

cf. https://code.visualstudio.com/docs/editor/extension-gallery#_workspace-recommended-extensions

If you don't think that adds any value, I'll drop it.

Yes, I am aware of the feature. I have always been against IDE specific files in the repository, but I do not mind if you want to keep it here.

I would personally remove it.

Dockerfile

XVincentX · 2020-04-27T20:31:23Z

src/index.ts

+            info(`Done linting '${v.path}'`);
+


Is it really worth to add logging here? It's a side effect and it should be encapsulated in an IO .

It would better to use a Writer or simply hold off on logging here

src/index.ts

XVincentX · 2020-04-27T20:35:28Z

src/spectral.ts

@@ -11,11 +13,28 @@ import {
  isOpenApiv3,
 } from '@stoplight/spectral';

+import * as SpectralPackage from '../node_modules/@stoplight/spectral/package.json';


This cannot be statically imported, because this might not exist. You need to replace this with an IOEither with a require call

@XVincentX I've tried to apply your hints (Not that they were unclear, but rather because I'm not really used to fp-ts. Hence "tried").

Could you please check that I've correctly understood the requirements?

src/spectral.ts

XVincentX · 2020-04-27T20:36:45Z

src/spectral.ts

+    if (rulesetPath.length !== 0) {
+      info(`Loading ruleset '${rulesetPath}'...`);
+      normRuleSet = [rulesetPath];
+    } else {
+      info(`Loading built-in rulesets...`);
+      normRuleSet = undefined;
+    }


Replace this with an expression so that normRuleSet is constant

@XVincentX Fixed. Do you like it better?

src/spectral.ts

nulltoken · 2020-04-28T09:54:28Z

@XVincentX I believe I've fixed all your comments. Could you please take another peek?

src/spectral.ts

XVincentX · 2020-04-28T15:47:56Z

@nulltoken

Please do not do this, it makes reviewing the follow up commits almost impossible and it's forcing the to re-review the whole PR.

philsturgeon

I can't approve this, but it would be good to get @XVincentX to take one last look anyway. I'll leave it with you two!

philsturgeon · 2020-04-28T15:56:49Z

@nulltoken btw can you push a v2 tag when this is done? We don't have semantic release set up or anything like that.

XVincentX · 2020-04-28T16:07:28Z

src/spectral.ts

+const retrieveSpectralPackageVersion = (): IOEither.IOEither<Error, string> =>
+  IOEither.tryCatch<Error, string>(() => {
+    const x = require('../node_modules/@stoplight/spectral/package.json');
+    return String(x.version);
+  }, E.toError);


You do not need to make a function of this

Suggested change

const retrieveSpectralPackageVersion = (): IOEither.IOEither<Error, string> =>

IOEither.tryCatch<Error, string>(() => {

const x = require('../node_modules/@stoplight/spectral/package.json');

return String(x.version);

}, E.toError);

const retrieveSpectralPackageVersion =

IOEither.tryCatch<Error, string>(() => {

const x = require('../node_modules/@stoplight/spectral/package.json');

return String(x.version);

}, E.toError);

@XVincentX Duh. I now realize I didn't take this into account. Sorry :-(

XVincentX

I will probably jump on this integration and clean up some stuff, but for now this is good to go.

nulltoken · 2020-04-28T16:26:01Z

@XVincentX It's in. Thanks a lot for your patience and your feedback! I definitely need to feel more at easy with fp-ts.

nulltoken · 2020-04-28T16:30:26Z

I will probably jump on this integration and clean up some stuff, but for now this is good to go.

@XVincentX Thanks. I'll read your changes to learn from them.

Once you're done, could you please take care of the tagging (as requested by @philsturgeon above)?

nulltoken · 2020-04-28T16:31:56Z

@nulltoken

Please do not do this, it makes reviewing the follow up commits almost impossible and it's forcing the to re-review the whole PR.

re PR-rewrite. Sorry, bad habit. Will keep the fixups! commits from now on.

philsturgeon mentioned this pull request Feb 25, 2020

Custom Rulesets in GitHub Action stoplightio/spectral#327

Closed

nulltoken force-pushed the custom-rulesets branch from 783e876 to 432d0f9 Compare April 25, 2020 06:47

nulltoken reviewed Apr 25, 2020

View reviewed changes

README.md Show resolved Hide resolved

nulltoken force-pushed the custom-rulesets branch from 45d8328 to 66b7178 Compare April 25, 2020 20:23

chore: add .editorconfig file

8d0d035

nulltoken force-pushed the custom-rulesets branch 8 times, most recently from 0655244 to 1faed38 Compare April 26, 2020 20:00

nulltoken marked this pull request as ready for review April 26, 2020 20:13

nulltoken requested review from XVincentX and P0lip April 26, 2020 20:14

nulltoken force-pushed the custom-rulesets branch 3 times, most recently from 20721b3 to 8b2b579 Compare April 27, 2020 08:47

XVincentX suggested changes Apr 27, 2020

View reviewed changes

nulltoken added 8 commits April 28, 2020 11:34

chore: clean up TypeScript build

74d47a3

chore: move dependencies under the proper package.json section

ab9f094

chore: do not require from the user to pass the GitHub token

63c1faa

chore: use fixed version of dependent actions

98edc5c

chore: move test file under a fixtures folder

e8ae798

feat: support custom rulesets

7fa7f6e

chore: enhance documentation

dafd2dc

fix: do not create the checkrun with the default 'queued' status

78bd149

nulltoken force-pushed the custom-rulesets branch from 0dcb3ec to ba5a777 Compare April 28, 2020 09:42

nulltoken requested a review from XVincentX April 28, 2020 09:54

XVincentX reviewed Apr 28, 2020

View reviewed changes

src/spectral.ts Outdated Show resolved Hide resolved

nulltoken added 5 commits April 28, 2020 17:38

chore: enhance logging

787c73b

chore: track a potential future enhancement

4bfd735

chore: fail the main action on fatal linting errors

a0b1317

fix: tie annotations to proper commit

b277ed1

fix: Have separate Lint checks for push and pull requests

13042a4

nulltoken force-pushed the custom-rulesets branch from ba5a777 to 13042a4 Compare April 28, 2020 15:39

nulltoken requested a review from XVincentX April 28, 2020 15:40

philsturgeon commented Apr 28, 2020

View reviewed changes

XVincentX reviewed Apr 28, 2020

View reviewed changes

XVincentX approved these changes Apr 28, 2020

View reviewed changes

nulltoken merged commit 5c59284 into master Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: custom rulesets #129

feat: custom rulesets #129

philsturgeon commented Feb 25, 2020

rubenfiszel commented Mar 26, 2020

philsturgeon commented Mar 26, 2020

rubenfiszel commented Mar 26, 2020

nulltoken commented Apr 25, 2020

nulltoken commented Apr 25, 2020 •

edited

Loading

nulltoken commented Apr 25, 2020

nulltoken commented Apr 25, 2020

nulltoken commented Apr 26, 2020

nulltoken commented Apr 26, 2020

nulltoken commented Apr 27, 2020

XVincentX commented Apr 27, 2020

rubenfiszel commented Apr 27, 2020

XVincentX Apr 27, 2020

nulltoken Apr 28, 2020

XVincentX Apr 28, 2020

XVincentX Apr 27, 2020

XVincentX Apr 27, 2020

nulltoken Apr 28, 2020 •

edited

Loading

XVincentX Apr 27, 2020

nulltoken Apr 28, 2020

nulltoken commented Apr 28, 2020

XVincentX commented Apr 28, 2020

philsturgeon left a comment

philsturgeon commented Apr 28, 2020

XVincentX Apr 28, 2020

nulltoken Apr 28, 2020

XVincentX left a comment

nulltoken commented Apr 28, 2020

nulltoken commented Apr 28, 2020

nulltoken commented Apr 28, 2020

		@@ -0,0 +1,5 @@
		{
		// See https://go.microsoft.com/fwlink/?LinkId=827846

feat: custom rulesets #129

feat: custom rulesets #129

Conversation

philsturgeon commented Feb 25, 2020

rubenfiszel commented Mar 26, 2020

philsturgeon commented Mar 26, 2020

rubenfiszel commented Mar 26, 2020

nulltoken commented Apr 25, 2020

nulltoken commented Apr 25, 2020 • edited Loading

nulltoken commented Apr 25, 2020

nulltoken commented Apr 25, 2020

nulltoken commented Apr 26, 2020

nulltoken commented Apr 26, 2020

nulltoken commented Apr 27, 2020

XVincentX commented Apr 27, 2020

rubenfiszel commented Apr 27, 2020

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

nulltoken Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Apr 28, 2020

XVincentX commented Apr 28, 2020

philsturgeon left a comment

Choose a reason for hiding this comment

philsturgeon commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

nulltoken commented Apr 28, 2020

nulltoken commented Apr 28, 2020

nulltoken commented Apr 28, 2020

nulltoken commented Apr 25, 2020 •

edited

Loading

nulltoken Apr 28, 2020 •

edited

Loading