Design For Linter - Githubissues

RubenHalman commented 3 years ago

Any design, maybe just pseudo code will help me forward.

How can/will a SFDX plugin be integrated in a deployment pipeline?
Currently the core module has a Scan function, which returns an Array. I assume the CLI plugin will throw an error based on these results, or return a list of results?
At least for now, I want the rule definitions to stay in the Core Module. Except for naming conventions, I dont see other use cases for extending the rules yet. Preferably the tooling provides the best standard possible which should be much less subjective versus with a traditional code base.
With the above in mind, it is even more important to have a way to configure what results should not be returned. Maybe certains flows need to be excempt or optimally, specific flow elements or rules within a flow. How would these configurations look like and work with the (Automated) cli tooling?

RubenHalman commented 3 years ago

@pozil I would like to pick your brain on this if your offer still stands!

pozil commented 3 years ago

Happy to help. Here are my thoughts:

this depends on the type of CI. The plugin can either be pre-installed by an admin or it can be installed as part of the CI workflow just like this.
my recommendation is use common CLI patterns: use the command exit code (non-zero means a failure) and the output stream. For the output stream, I recommend the same kind of format as apex tests: a -f or --format flag that lets the user toggle between human or json. This way, CI will most likely only check for the exit code but it can also do something with JSON if needed (nice to have but cheap to implement) and the user can look at a nicely formatted output stream.
yes, I agree that rule definitions belong in core.
I recommend the same kind of config that ESLint uses: one or more JSON configuration files that are left in the DX project and versioned (this way, CI will also get it) just like force-app/main/default/lwc/.eslintrc.json. These files should be used to configure rules and exceptions. When the flow health check plugin runs, it will inspect all **/flows folders (there could be more than one) and load relevant config. If the configuration file becomes too complex because of the exceptions, you could also support configuration files at the flow level. For example, you could have an optional myFlow.lintconfig file where myFlow is the name of the flow (right next to the myFlow.flow-meta.xml file).

RubenHalman commented 3 years ago

@pozil Thank you, this helps a lot!

I have one question about the fourth point: this means the sfdx plugin will require a project, but would it be possible to support both: providing either a username or having a local project? Also, looking at the sfdx example, this seems to be not possible ootb.

Actually, I am already having doubts if that will be valuable

pozil commented 3 years ago

SFDX plugins can run with or without a project or username.

For a given command you can set a bunch of flags from SfdxCommand by overriding them:

protected static supportsUsername: boolean;
protected static requiresUsername: boolean;
protected static supportsDevhubUsername: boolean;
protected static requiresDevhubUsername: boolean;
protected static requiresProject: boolean;

For example, this custom plugin command doesn't require a project.

RubenHalman commented 3 years ago

@pozil Thank you for your help! Much to your credit I got the first working version of the CLI extension, but I am hoping you can confirm if you consider it usable yet or not. Running it should be straightforward: Install: npm install -g lightningflowscan-cli Use: sfdx scan:flowscan

In the repository that contains example flows I made a PR demonstrating how you would be able to ignore results by flow or by a combination of flow and rule. I am certainly trying to think of a better solution here and if you have suggestions, they will be very welcome.

The core module can be found here

pozil commented 3 years ago

I'm looking into this but there are some issues. I got it to install correctly. I'm using the CLI installed with the installer so that's: sfdx plugins:install lightningflowscan-cli Then, I tried to run the scan but the command is: sfdx scan:flows (not flowscan) Finally, I'm getting this error:

ERROR running scan:flows:  core.scan is not a function

The version that's reported with sfdx plugins is 0.0.8.

RubenHalman commented 3 years ago

@pozil My apologies. This might be due to version of the core that you have installed(I am working on a newer version) if you run yarn upgrade or checkout specific branches

Try installing the 0.0.6 specific version on your device like so: npm install -g lightningflowscan-cli@0.0.6

Also, make sure the command is run within a sfdx project.

I will look into this ASAP and signal you when the newest versions are also working across

pozil commented 3 years ago

No luck:

➜  easy-spaces-lwc git:(main) ✗ sfdx plugins:install lightningflowscan-cli@0.0.6
This plugin is not digitally signed and its authenticity cannot be verified. Continue installation y/n?: y
Finished digital signature check.
warning "@oclif/plugin-autocomplete > @oclif/command@1.5.19" has unmet peer dependency "@oclif/plugin-help@^2".
warning "@salesforce/plugin-functions > yeoman-environment@3.4.1" has unmet peer dependency "mem-fs@^1.2.0 || ^2.0.0".
warning "@salesforce/plugin-functions > yeoman-environment@3.4.1" has unmet peer dependency "mem-fs-editor@^8.1.2 || ^9.0.0".
Installing plugin lightningflowscan-cli... installed v0.0.6
➜  easy-spaces-lwc git:(main) ✗ sfdx scan:flows
ERROR running scan:flows:  core.scan is not a function

I'm wondering if dependencies are properly packaged in the binary...

RubenHalman commented 3 years ago

Thank you for trying and my apologies for the inconvenience. For whatever reason it does seem to work for MacOS based devices and I am only able to reproduce this on windows machines. I will try to resolve it ASAP and keep you posted, it might take me several days.

pozil commented 3 years ago

No worries and no rush. Thanks for building this cool tool.

RubenHalman commented 3 years ago

Actually I had a lucky shot. I am not sure exactly what the issue was, but after reading about known issues I decided to use plugins:generate to generate the set up files from scratch and everything works as expected on multiple devices now.

I think you do need to uninstall the plugin first like so:

afbeelding

RubenHalman commented 3 years ago

Also, I messed up your PR and reverted some of the eslint specific changes of your PR. Sorry! Original pr Overwriting changes

pozil commented 3 years ago

@RubenHalman are you still interested in those changes, should I bring them back?

RubenHalman commented 3 years ago

That is okay, I need to find out why tslint was still required on packaging but other then that I think most of your changes are still there. Thank you for your contributions, I am most curious to what degree you find the solution workable! I am currently just throwing errors with exit code '1' without much details such as line number in the files etc.

pozil commented 3 years ago

Regarding the PR, the problem might have been that I removed tsconfig.json.

I just tried to reinstall the plugin (getting version 0.0.15) and running it on the easy-spaces-lwc sample app. I didn't get an error this time but I also got no output at all: the command ended silently. Is that expected?

Also, the help flag seems to be bound to an incorrect message for the description.

➜  easy-spaces-lwc git:(main) ✗ sfdx scan:flows
➜  easy-spaces-lwc git:(main) ✗ sfdx scan:flows -h
Fix the following issues:

USAGE
  $ sfdx scan:flows [--json] [--loglevel trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL]

OPTIONS
  --json                                                                            format output as json
  --loglevel=(trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL)  [default: warn] logging level for this command invocation

 ›   Error: EEXIT: 0
 ›   Code: EEXIT

RubenHalman commented 3 years ago

Yes, I think that is to be expected as the easy-spaces-lwc app does not have any flows or at least non are violating any rules. Another example that you should be able to run the linter without results is the dreamhouse app, where Alba Rivas made some improvements to the create property flow based on the vsce outcomes. I can include more useful information in the results, but I would like to verify with you if the outcome would be sufficient for implementing the sfdx plugin in automated builds as is. If you want examples of rule violations you can have a look at the example repository.

With regards to tsconfig.json, it seems you are right, but I am wondering why it would be a problem of if sfdx plugins need to use tslint.

pozil commented 3 years ago

Re tsconfig.json: this file is not related to tslint, it's used by the typescript compiler so we ~can~ should get rid of tslint but we need that config file back.

Thanks for your feedback I was able to run the CLI also on Dreamhouse. Some feedback on the output:

Let's have some some summary info in the output:

in 'human' output mode, we could have something like this so that we know that the scan worked (so that people don't get confused about no output like I was and so that they can also make sure that the number of flows match what they expect):
```
Scanned 3 flows and found no issues.
```
OR
```
Scanned 3 flows and found 1 issue(s).
```

in JSON output mode, I recommend that you add a summary property with the same info plus adopt a more programmatic approach to the list of errors and their description:

{
...
"summary": {
  "flowCount": 3,
  "errorCount": 1
},
"errors": [
  {
     "flow": "Create Property",
     "rule": "RuleName",
     "error": "Some human-friendly description of the error"
  }
]
}

Ideally this info could be read by another tool like JQ or something else in a CI workflow and people could build some sort of health report of an org automatically. Ideally, you would use the same kind of info to render the view in your VS Code extension.

Let me know if this makes sense.

RubenHalman commented 3 years ago

Thank you, this is what I needed to know and it makes sense. The sfdx throw error only takes an array of strings, so I am actually not sure how exactly it should work. I believe If you are using something like JQ, the output does not necessarily need to be on the last line, and it maybe allows me me to return a JSON followed an error. I will start with the JSON output, buI was also looking into things like the sfdx flag: --json Suppresses output from this.ux.* methods, and formats output as JSON. This flag is always enabled on all Salesforce CLI commands. Its value is treated as true if provided, false otherwise.

Should a successful result look something like this:

  "summary": {
    "flowCount": 3,
    "errorCount": 0
  },

One other thing I am not happy about is how ignoring certain results works. Your previous suggestion using files per flow seems like a good approach but I am trying to think about what different types of rules might be requiring files.

RubenHalman commented 3 years ago

@pozil I believe I have achieved exactly this(although I had to limit the amount of detailed errors to 5).

Please have a look at the readme to install the new version and please do let me know your thoughts!

Lightning-Flow-Scanner / lightning-flow-scanner-sfdx

Design For Linter #2