-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add exclude-paths option to dependabot.yml #12532
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
Add exclude-paths option to dependabot.yml #12532
Conversation
bf36625
to
696c6c6
Compare
@dnlfm You may like to fix the specs which are failing. |
@randhircs I'm sorry but why did you force push, rewriting previous commits? The failing spec is not related to the changes in this PR: It seems this issue appeared after your commit. Force-pushes should be avoided, it may have modified something that it is hard to know what it is, as it may have rewritten the git tree. The |
@dnlfm I had to rebase to proceed to check if all specs are passing. I am sure sorbet fix is something you can check from your end. |
696c6c6
to
95d602e
Compare
@dnlfm Again now, I am checking by rebasing it to see if all specs are good. i feel that you may like to fix the sorbet issue. |
Hi @randhircs,
The file I would like to also ask you to remove the need of Sorbet typing in test files, as I added I would prefer to not have to check these Sorbet issues by myself. |
@dnlfm — We actually added full Sorbet type checks later on. However, files without proper Sorbet typings won’t be caught by the pipeline unless they’re modified. Once someone touches such a file, it becomes a requirement to fully type it, with the goal of gradually achieving complete Sorbet coverage. If you're able to add the Sorbet typings here, that would be great. But if it feels like too much, @randhircs can help with that. Thanks again for your contribution! @randhircs — You might be able to help here. Also, if you're planning to push additional commits, please avoid force-pushing, as we’d like to retain @dnlfm’s original commit. |
Hi @kbukum1, Thank you for your response.
I suspected that could be the case, but I would encourage a different approach: I would suggest that the team responsible for adding Sorbet could map the files that they need to go through and address each one of them, let's say, 40 files a week, prioritizing the ones in PRs that are already open. This way, PRs will have isolated changes, i.e. the PRs won't be touching something that is not meaningful to their changes, and thus making the code review process better. Also, I would really appreciate if a code review is done on what I have done. I looked at my PR once again today, and I started wondering if what I have done will work for all kind of repositories or if there is something missing, for instance:
|
@dnlfm, Also, we are planning on releasing this as a new feature. With regards to this, we would need to add a feature flag which when enabled, these changes would be in effect. I will be creating the feature flag shortly. Please let me know if you prefer adding the feature flag, which I can share with you, in the code or I can push those changes myself to the same PR. Thanks! |
Hi @AbhishekBhaskar, that's great and thank you for asking! Please feel free to push the changes directly to this PR. |
Thank you, @randhircs, fixed it. |
|
…m:dnlfm/dependabot-core into dnlfm/exclude-directories-flag-issue-4364
…m:dnlfm/dependabot-core into dnlfm/exclude-directories-flag-issue-4364
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@linked_paths = T.let({}, T::Hash[T.untyped, T.untyped]) | ||
@submodules = T.let([], T::Array[T.untyped]) | ||
@options = options | ||
|
||
@files = T.let([], T::Array[DependencyFile]) | ||
end | ||
|
||
# rubocop:disable Style/TrivialAccessors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using disabling rubocop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbhishekBhaskar Could you try removing the RuboCop:disable
@@ -453,14 +462,18 @@ def codecommit_client | |||
params(path: String, fetch_submodules: T::Boolean, raise_errors: T::Boolean) | |||
.returns(T::Array[OpenStruct]) | |||
end | |||
def _fetch_repo_contents(path, fetch_submodules: false, | |||
raise_errors: true) | |||
def _fetch_repo_contents(path, fetch_submodules: false, raise_errors: true) # rubocop:disable Metrics/PerceivedComplexity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may create new method and remove this # rubocop:disable.
@@ -541,6 +554,66 @@ def _cloned_repo_contents(relative_path) | |||
size: 0 # NOTE: added for parity with github contents API | |||
) | |||
end | |||
if Dependabot::Experiments.enabled?(:enable_exclude_paths_subdirectory_manifest_files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to split method to limit the method size.
This PR adds the possibility of specifying
exclude-paths
in dependabot.yml.It's a very old issue, and I decided to give a first stab: #4364
End-to-end tests may be required to validate the solution. Unit tests are in place.
Example of dependabot.yml with the new option: