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

dry-run: Dependabot::ConfigFile #3512

Merged
merged 12 commits into from
Apr 20, 2021
Merged

dry-run: Dependabot::ConfigFile #3512

merged 12 commits into from
Apr 20, 2021

Conversation

thepwagner
Copy link
Contributor

@thepwagner thepwagner commented Apr 19, 2021

This introduces the Dependabot::Config module, starring the Dependabot::Config::File class.

This PR wraps the configuration stored in dependabot.yaml, with helpers to locate the configuration relevant to the active ecosystem/directory.

This is incomplete, only extracting the features to provide:

@thepwagner thepwagner self-assigned this Apr 19, 2021
@thepwagner thepwagner requested a review from a team as a code owner April 19, 2021 14:49
Copy link
Contributor

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

Nice! I also started on some of these primitives a while ago and wondering if we want to crib some stuff from there?

I tried to implement the config file fetcher as another file fetcher: https://github.com/dependabot/dependabot-core/compare/feelepxyz/cli-experiment?expand=1#diff-7f068f9118efc60b552a73767f52e71a60020c0690b239c292b4fc050d421782R8-R42

I also copied over the whole schema and a lot of the parsing logic which we probably don't want to get this over the line.

@thepwagner
Copy link
Contributor Author

thepwagner commented Apr 19, 2021

I tried to implement the config file fetcher as another file fetcher

SGTM, I'll make this change.

It's definitely a chicken/egg problem: the FileFetcher instance is informed by the package_manager, but the active package_manager s are contained the config file.

copied over the whole schema and a lot of the parsing logic which we probably don't want to get this over the line

I wasn't keen to bring "real" validation (e.g. the JSON schema), it seems harmless scope creep but would have the maintenance cost of requiring the schema be maintained in two places.
There's some demand, e.g. #1957 #1927 - I just didn't want to go there (yet).

@thepwagner thepwagner mentioned this pull request Apr 19, 2021
Copy link
Contributor

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

LGTM overall, left a few minor comments, feel free to merge when you've folded in what makes sense 👍

Copy link
Contributor

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

🙌

@thepwagner thepwagner merged commit 0352d54 into main Apr 20, 2021
@thepwagner thepwagner deleted the dry-run-config-file branch April 20, 2021 14:37
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