Add deprecation message to the created pull request #10379
Conversation
kbukum1
force-pushed
the
kamil/bundler_v1_deprecation_warning_on_pr_description
branch
5 times, most recently
from
f08fcd6
to
4b44e8b
Compare
kbukum1
force-pushed
the
kamil/bundler_v1_deprecation_warning_on_pr_description
branch
from
1d93c23
to
c7d7fa5
Compare
kbukum1
force-pushed
the
kamil/bundler_v1_deprecation_warning_on_pr_description
branch
3 times, most recently
from
8ac3f05
to
5b24911
Compare
added generating notices structure. added generating deprecation and unsupported notices. added deprecation warning as PR notice.
kbukum1
force-pushed
the
kamil/bundler_v1_deprecation_warning_on_pr_description
branch
from
5b24911
to
81300b9
Compare
There was a problem hiding this comment.
So we're on the same page, at a high level, the way an update works is:
- FileFetcher
- FileParser
- For each dependency:
a. UpdateChecker
b. FileUpdater - For each PR created: Gather PR body content
Of course this gets a little more complicated when multi-dir is involved. Because of multi-dir, I think having a single PackageManager that records the version is actually out of the question here. It's possible to have one directory be on a different version of the package manager than another.
So generally my thoughts for structuring this change:
- Move the existing ecosystem_version parsing to FileParser where it makes the most sense.
- Standardize on what they return (the PackageManager structure you have reconciled with what NPM already has)
- When processing a directory, POST to the ecosystem version endpoint, and if the version is not supported then skip the directory with a message
- Pass the directory's PackageManager into DependencySnapshot where it can tell the MessageBuilder to add the deprecation message.
| UNSUPPORTED_BUNDLER_VERSIONS = T.let([].freeze, T::Array[String]) | ||
| DEPRECATED_BUNDLER_VERSIONS = T.let(["1"].freeze, T::Array[String]) | ||
|
|
||
| class PackageManager < PackageManagerBase |
There was a problem hiding this comment.
There's a PackageManager class under the NPM ecosystem that doesn't look much like this one. Are we going to reconcile the differences?
| @@ -116,8 +118,19 @@ def conflicting_dependencies | |||
| ) | |||
| end | |||
|
|
|||
| sig { returns(PackageManagerBase) } | |||
| def package_manager | |||
There was a problem hiding this comment.
We already have a ecosystem_versions on the FileFetcher that we use to POST the ecosystem versions to the API to gather usage statistics.
We should either reuse that functionality or move it into the FileParser which might be the best place for it, and perhaps reconcile the differences between what they all return since they're all doing the same thing.
| @@ -484,6 +484,64 @@ def self.helper_subprocess_bash_command(command:, stdin_data:, env:) | |||
| env_keys = env ? env.compact.map { |k, v| "#{k}=#{v}" }.join(" ") + " " : "" | |||
| "$ cd #{Dir.pwd} && echo \"#{escaped_stdin_data}\" | #{env_keys}#{command}" | |||
| end | |||
|
|
|||
| sig do | |||
There was a problem hiding this comment.
These changes seem out of place in this file. It seems a more appropriate place would be in the PackageManagerBase or in the PullRequestCreator where most of the PR body generation exists.
| @@ -209,8 +215,32 @@ def ignore_requirements | |||
| ignored_versions.flat_map { |req| requirement_class.requirements_array(req) } | |||
| end | |||
|
|
|||
| sig { returns(T::Array[Notice]) } | |||
There was a problem hiding this comment.
Also seems out of place to have this plumbing in the UpdateChecker.
|
The feature implemented in the following PR, #10421 |
kbukum1
deleted the
kamil/bundler_v1_deprecation_warning_on_pr_description
branch
What are you trying to accomplish?
Implementing a feature to add deprecation messages to the created pull requests. This includes using log levels to match messages shown on the UI, ensuring clear communication of deprecation and other important information.
What issues does this affect or fix?
This enhancement adds the bundler v1 deprecation warning into the PR description.
Anything you want to highlight for special attention from reviewers?
deprecationwarning and attaching it to the pr_notices which can be used to attach to the PR. We are attaching generated notices here: Attach generated_pr_notices into the PR description #10399.PackageManagerBaseabstract class and createdPackageManagersubclass for bundler to make the package manager information reachable throughdependabot-coreupdaterandcommoncode. Currently, we are using this class for deprecation and support but it can be used for other purposes as well.How will you know you've accomplished your goal?
Checklist