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

Config option to allow pinned packages #84

Merged
merged 4 commits into from
Apr 6, 2022

Conversation

ivk1800
Copy link
Contributor

@ivk1800 ivk1800 commented Apr 3, 2022

Motivation

fix #83
some projects using pinned packages because:

  1. more version control
  2. prevent issues with implicit version update, as example [analyzer] InconsistentAnalysisException: Requested result might be inconsistent with previously returned results  dart-lang/sdk#48658

Changes

add new option for dart_dependency_validator.yaml

Release Notes

  • Feature: Added option allow_pins to tell dependency_validator to not fail on any pinned packages

Closes #83

@aviary2-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

Copy link
Contributor

@evanweible-wf evanweible-wf left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! I just have one suggestion about the name, but otherwise this looks good.

README.md Outdated
@@ -39,6 +39,8 @@ things in a `dart_dependency_validator.yaml` file in the root of your package:
```yaml
# dart_dependency_validator.yaml

# Set true if you use pinned dependencies
ignored_pinned_packages: true
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you think about naming this allow_pins? Given that this config will be in a file specific to dependency_validator, it should be clear from context that it's referring to pinned dependencies/packages, and I'd like to avoid the word "ignore" here because we're not entirely ignoring dependencies that are pinned, we're just disabling checks for pins. We would still flag other types of dependency warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, originally wanted use 'allow' for this

'''))
]).create();
result = checkProject('${d.sandbox}/dependency_pins');
expect(result.exitCode, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should update this test to run on a project with a pubspec that has a dependency pin but no other issues, and then verify that the exit code is 0. Would probably also need to add a file that uses the dependency so it doesn't fail on an unused dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

evanweible-wf
evanweible-wf previously approved these changes Apr 6, 2022
@evanweible-wf evanweible-wf changed the title ignored pinned packages Config option to allow pinned packages Apr 6, 2022
@evanweible-wf
Copy link
Contributor

QA +1

  • CI passes and added test adequately covers the new behavior

@Workiva/release-management-p

@chrisgustavsen-wf
Copy link

RM +1

Copy link
Contributor

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rmconsole2-wf rmconsole2-wf merged commit 4128ddf into Workiva:master Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config // allow pinned dependencies
6 participants