Implement Logging for Bundler v1 Deprecation Warnings

[kbukum1]

What are you trying to accomplish?

This PR introduces a deprecation warning logging for bundler v1.

Why:

• To provide information regarding deprecation in the logging for users.

This change ensures users are informed when using deprecated versions of Bundler and provides a structured approach for handling similar deprecations in other ecosystems.

Anything you want to highlight for special attention from reviewers?

The changes are focused on showing notices in the log. Especially this implementation adds structure to log notices and show bundler v1 deprecation warning in the log.

How will you know you've accomplished your goal?

• Successfully logging deprecation warnings for Bundler v1.
• Verification through tests that the correct deprecation warning shown for bundler v1
• Similar to the following deprecation warnings should appear in the log when job runs on bundler v1.

Example warning

2024/08/26 20:14:33 WARN Dependabot will stop supporting `bundler v1`!
2024/08/26 20:14:33 WARN Please upgrade to version `v2`.

Example log when updating job on bundler v1.

warning: parser/current is loading parser/ruby33, which recognizes 3.3.4-compliant syntax, but you are running 3.3.1.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
2024/08/26 20:14:33 INFO Starting job processing
2024/08/26 20:14:33 WARN Dependabot will stop supporting `bundler v1`!
2024/08/26 20:14:33 WARN Please upgrade to version `v2`.
2024/08/26 20:14:34 INFO Starting update job for dsp-testing/updates-issues-10421
2024/08/26 20:14:34 INFO Checking all dependencies for version updates...
2024/08/26 20:14:34 INFO Checking if tzinfo  needs updating
2024/08/26 20:14:35 INFO Latest version is 2.0.6
2024/08/26 20:14:36 INFO Requirements to unlock update_not_possible
2024/08/26 20:14:36 INFO Requirements update strategy bump_versions_if_necessary
2024/08/26 20:14:36 INFO No update possible for tzinfo 
2024/08/26 20:14:36 INFO Checking if google-protobuf  needs updating
2024/08/26 20:14:36 INFO Filtered out 380 pre-release versions
2024/08/26 20:14:36 INFO Latest version is 4.27.3
2024/08/26 20:14:38 INFO Requirements to unlock own
2024/08/26 20:14:38 INFO Requirements update strategy bump_versions_if_necessary
2024/08/26 20:14:38 INFO Updating google-protobuf from  to 4.27.3
2024/08/26 20:14:38 INFO Submitting google-protobuf pull request for creation
    cli | 2024/08/26 20:14:41 yaml: unmarshal errors:  line 1: field directory not found in type model.Dependency
                                                                                                                 2024/08/26 20:14:41 INFO Finished job processing
2024/08/26 20:14:41 INFO Results:
+-----------------------------------------------+
|      Changes to Dependabot Pull Requests      |
+---------+-------------------------------------+
| created | google-protobuf ( from  to 4.27.3 ) |
+---------+-------------------------------------+

The following

Checklist

• I have run the complete test suite to ensure all tests and linters pass.
• I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
• I have written clear and descriptive commit messages.
• I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
• I have ensured that the code is well-documented and easy to understand.

[kbukum1]

The following up PR needs this PR to be done.

• Sending Bundler v1 Deprecation Warning Alert #10485

[abdulapopoola]

Taggin @Nishnha and @honeyankit for reviews

[kbukum1]

Taggin @Nishnha and @honeyankit for reviews

Thanks.

[kbukum1]

@honeyankit
FF is defined in the previous PR, #10421 which was adding deprecation notice to the PR message and the FF can be found here. Just moved the location in this PR.

https://github.com/dependabot/dependabot-core/pull/10466/files#diff-91b7c54162fa653b64cba3cf2725dd2dca170a6c96074791ca12385544693467R233

[Nishnha]

Hi it looks like this PR renames a few fields, like message->description and introduces new fields like title and NoticeMode.

I was wondering how these new fields are used? Was the message field not working properly? Also, where does the title get used?

I also noticed that this introduces a lot of code to say exactly what versions of the package manager users can upgrade to. But it might just be easier to say "Please upgrade your package manager version" without specifying the versions themselves.

Finally, would it make more sense to display the deprecation notice at the start of the job log instead of after gathering the requirements to unlock?

[kbukum1]

Hi it looks like this PR renames a few fields, like message->description and introduces new fields like title and NoticeMode.

I was wondering how these new fields are used? Was the message field not working properly? Also, where does the title get used?

Hi @Nishnha ,

This PR is continuation of the previous completed task, https://github.com/dependabot/dependabot-core/pull/10421/files. We just started to have notice structure for showing Notice. So before it is defined as message however that's was confussing since we are going to show this information in alert body description. So That's why it is renamed after discussing with @abdulapopoola.

Title is also going to be show on top of the warning alert as a description on insight page. Normally we are generating title on dependabot-api for errors but this causing to always make changes on dependabot-api to add new errrors. So I made decision to add title to notices that can be completely managable from dependabot-core instead of dependabot-api.

I also noticed that this introduces a lot of code to say exactly what versions of the package manager users can upgrade to. But it might just be easier to say "Please upgrade your package manager version" without specifying the versions themselves.

For that I discussed that with @jonjanego. If package manager class doesn't have the supported list basicly we are not showing this message and just sharing the warning. If the list is defined then we are showing this message.

Finally, would it make more sense to display the deprecation notice at the start of the job log instead of after gathering the requirements to unlock?

The reason of not showing at the start was because I am showning all warnings (notices) when we are ready to create PR. For deprecation it may be possible to show at start. However I thought it is ok not showing warning when there is error or PR is up-to-date.

[Nishnha]

This PR is continuation of the previous completed task, https://github.com/dependabot/dependabot-core/pull/10421/files. We just started to have notice structure for showing Notice. So before it is defined as message however that's was confussing since we are going to show this information in alert body description. So That's why it is renamed after discussing with @abdulapopoola.

Title is also going to be show on top of the warning alert as a description on insight page. Normally we are generating title on dependabot-api for errors but this causing to always make changes on dependabot-api to add new errrors. So I made decision to add title to notices that can be completely managable from dependabot-core instead of dependabot-api.

Oh okay I didn't realize it would also show in alert descriptions. That makes sense!

I also noticed that this introduces a lot of code to say exactly what versions of the package manager users can upgrade to. But it might just be easier to say "Please upgrade your package manager version" without specifying the versions themselves.

For that I discussed that with @jonjanego. If package manager class doesn't have the supported list basicly we are not showing this message and just sharing the warning. If the list is defined then we are showing this message.

Got it - that seems like a roundabout way of doing it though. I think it would be easier to check if the package manager version needs the deprecation warning in Dependabot API, and if it does, pass that warning message along with the job details.

Feel free to push back on this though. I know you already have a lot of the plumbing set up in Core.

Finally, would it make more sense to display the deprecation notice at the start of the job log instead of after gathering the requirements to unlock?

The reason of not showing at the start was because I am showning all warnings (notices) when we are ready to create PR. For deprecation it may be possible to show at start. However I thought it is ok not showing warning when there is error or PR is up-to-date.

I think it would be okay to always show the warning for a package manager that is about to be deprecated. I also think it's more likely to be seen at the top of a job log.

[Nishnha]

Got it - that seems like a roundabout way of doing it though. I think it would be easier to check if the package manager version needs the deprecation warning in Dependabot API, and if it does, pass that warning message along with the job details.

Feel free to push back on this though. I know you already have a lot of the plumbing set up in Core.

Chatted with @kbukum1 on this and we actually do not have all the package manager version information until the job runs. So this has to be done in core after the file parser runs.

[kbukum1]

@Nishnha

The problems we discussed are solved. Here is the last screenshot.

Problems:

• Moving logging warning notice on top of the job.
• Removed markdown field and added markdown generator method that is used in pr message generation.

Screenshot

[Nishnha]

Nice this is a lot easier to read! I see a failing CI test but I think it's unrelated to this PR?

I'm happy to approve this PR as-is, but I do notice a few things that we could improve before merging.

In the generate_supported_versions_description method, why is the support_later_versions? parameter used? I'm not familiar with the method so I was wondering when does Dependabot say it supports later versions instead of explicitly stating which versions it supports?

Another thing is, can we add the deprecation notice to the logs in 1 spot instead of adding it to all 4 operations? If we can, I think it would be easier to maintain!

[kbukum1]

@Nishnha ,

Thanks for the review.

Nice this is a lot easier to read! I see a failing CI test but I think it's unrelated to this PR?

I'm happy to approve this PR as-is, but I do notice a few things that we could improve before merging.

In the generate_supported_versions_description method, why is the support_later_versions? parameter used? I'm not familiar with the method so I was wondering when does Dependabot say it supports later versions instead of explicitly stating which versions it supports?

I think that is going to be hard because we don't know package manager until we are in the operation loop. Only one place is possible which is creation of dependency_snapshot but I am thinking that is not right place to do operation related to notice.

[Nishnha]

Another thing is, can we add the deprecation notice to the logs in 1 spot instead of adding it to all 4 operations? If we can, I think it would be easier to maintain!

I think that is going to be hard because we don't know package manager until we are in the operation loop. Only one place is possible which is creation of dependency_snapshot but I am thinking that is not right place to do operation related to notice.

@kbukum1 and I had a sync to discuss this and it does seem like it would be difficult to add the notices/warnings anywhere else. It also makes sense to keep the notices in the operations because as the job is processed, we may discover more things that we want to add a notice for, and the notice may differ for each operation.

So duplicating the notices in each of the operations is intentional 👍🏾

[kbukum1]

Another thing is, can we add the deprecation notice to the logs in 1 spot instead of adding it to all 4 operations? If we can, I think it would be easier to maintain!

I think that is going to be hard because we don't know package manager until we are in the operation loop. Only one place is possible which is creation of dependency_snapshot but I am thinking that is not right place to do operation related to notice.

@kbukum1 and I had a sync to discuss this and it does seem like it would be difficult to add the notices/warnings anywhere else. It also makes sense to keep the notices in the operations because as the job is processed, we may discover more things that we want to add a notice for, and the notice may differ for each operation.

So duplicating the notices in each of the operations is intentional 👍🏾

As @Nishnha mentioned we added notices list for each operations so that it can be used to create new notices in any stage of the operation. Also we added generation of deprecation notice in the dependency_snapshot because that is the unique before starting operation to get package manager information. Also since when we are using dependency snapshot we need workspace to be set (@current_directory), we decided to log deprecation notice here after parsing the dependency files.

[kbukum1]

Here is the last screenshot. Warning is showing after starting the process.