Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: pull out custom rules code #2751

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

teodora-sandu
Copy link
Contributor

@teodora-sandu teodora-sandu commented Feb 11, 2022

What does this PR do?

This PR extracts the custom and internal Snyk rules code from the main test function into its own file. For increased readability and in preparation of adding extended Terraform language support, which will make this function even bigger. This is needed for #2752 (which is not ready for review yet and pending snyk/snyk-iac-parsers#7).

How should this be manually tested?

To review, have a look at the new initRules function and notice how it matches up with the code extracted from the test function.

To test it:

  1. npm run build
  2. Create a custom rules bundle using https://github.com/snyk/snyk-iac-rules: snyk-iac-rules build ./fixtures/custom-rules -o bundle.tar.gz
  3. Run the CLI on a file that would trigger the rule (just use the fixtures in that repo): snyk-dev iac test ./fixtures/custom-rules/rules/CUSTOM-1/fixtures

The acceptance tests would verify this plus the remote bundles, so if the tests pass then this is good to go.

@teodora-sandu teodora-sandu requested a review from a team as a code owner February 11, 2022 19:41
@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

Warnings
⚠️

You've modified files in src/ directory, but haven't updated anything in test folder. Is there something that could be tested?

Messages
📖

This PR will not trigger a new version. It doesn't include any commit message with feat or fix.

Generated by 🚫 dangerJS against e791e19

Copy link
Contributor

@ofekatr ofekatr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent initiative, the main flow feels much cleaner after this refactor.
Managed to successfully run it locally 🚀
Left a small refactor suggestion open for discussion.

iacOrgSettings: IacOrgSettings,
options: IaCTestFlags,
): Promise<{
isLocalCustomRules: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a request, but rather an idea open for discussion.

I've given a bit of consideration to the return type of this function.
What we have here seems to be a tri-state value for provided custom rules.
Custom rules are either:

  1. Not provided.
  2. Provided locally.
  3. Provided remotely.

Instead of returning an object with 2 boolean flags, one for flagging rules provided locally, and one for flagging provided remotely, I'd like to propose returning a union type value here:

type RulesOrigin = 'local' | 'remote' | 'none';

Which can later be utilized in the addIacAnalytics function, where these values are needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, the return value can be optional for the case where rules are not provided, and so the union values could be shortened to be:

type RulesOrigin = 'local' | 'remote';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a really good suggestion. Makes the code much easier to ready 🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants