-
Notifications
You must be signed in to change notification settings - Fork 904
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
(#1092) Incorporate beforemodify changes into develop branch #3065
Conversation
f4a943d
to
cdee930
Compare
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.
Overall it looks good.
For the tests, would it be worthwhile to check that the beforemodify scripts are not run more than once?
And to have a test that the beforemodify script of a new dependency is not run during upgrade (maybe this is already covered). So if packageA
v1.0.0 has no dependencies, but v2.0.0 has a dependency on packageB
(which is not installed), and packageB
has a beforemodify script.
I believe I have tests verifying that the beforeModify scripts are only run for the currently-installed version and not the version that is installed. If we want to explicitly add a test that makes sure we aren't running them multiple times we can do that as well. The latter suggestion is definitely worth adding, though, good idea. 🙂 |
This addition is necessary, as many occasions may arise where installing one package involves updating an already-installed dependency. In these cases we should still be running a beforeModify script if it's present, and this facilitates that. (cherry picked from commit 3a30cbc)
This test added ensures that when we are upgrading a package, both its own and any dependencies' beforeModify are run so that we can properly update dependencies which might need to shut down a long-running service before being upgraded. (cherry picked from commit 49cc8d7) # Conflicts: # src/chocolatey.tests.integration/chocolatey.tests.integration.csproj # src/chocolatey.tests.integration/scenarios/InstallScenarios.cs # src/chocolatey.tests.integration/scenarios/UninstallScenarios.cs # src/chocolatey.tests.integration/scenarios/UpgradeScenarios.cs
Changes originally in 9f5d9cc, adapted for the current develop branch and the changes therein. Additionally, replaced and obsoleted some overloads of public (why?) methods that have unused method arguments and made them protected.
cdee930
to
ea7fe9c
Compare
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!
@vexx32 thanks for getting this brought over from the support branch! |
Description Of Changes
I may have snuck in a few extra bits where I obsoleted some method overloads that probably should not be
public
and/or had arguments they weren't using and replaced them with a call to a new overload that isprotected
where possible and/or doesn't ask for arguments it doesn't need. Sue me, those bothered me a lot. I'm not sure we strictly need to obsolete them first given how much else we're also breaking, but it wouldn't majorly hurt at this point to do so.Motivation and Context
This is being pulled in so that we can ensure we keep feature parity with the v1.x branch as much as possible.
Testing
Operating Systems Testing
Tested on win11
Change Types Made
Change Checklist
Related Issue