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

(#886, 2693) Update packages.config install logging and add back resolve or load method for Assembly Resolution #2836

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

corbob
Copy link
Member

@corbob corbob commented Sep 28, 2022

Description Of Changes

Update the debugging output for the install from packages.config file to clearly delineate the start and end of the configuration output.

Add back the original definition of resolve_or_load_assembly to AssemblyResolution.

Motivation and Context

The output of the Configuration when Licensed Extension is installed is different from when just Open Source is in play. As such, the tests for the new functionality was failing. This updates the output to enable us to better test the command.

Chocolatey GUI fails to load with the update to resolve_or_load_assembly, so added back the original definition.

Testing

  1. Downloaded build artifacts from build server and installed on VM
  2. Installed Chocolatey GUI
  3. Launched Chocolatey GUI
  4. Confirmed it launched successfully
  5. Installed Licensed Extension
  6. Ensured the Pester tests for InstallCommand completed successfully.

Change Types Made

  • Bug fix (non-breaking change)
  • Feature / Enhancement (non-breaking change)
  • Breaking change (fix or feature that could cause existing functionality to change)
  • PowerShell code changes.

Related Issue

Fixes #886
Fixes #2693

Change Checklist

  • Requires a change to the documentation
  • Documentation has been updated
  • Tests to cover my changes, have been added
  • All new and existing tests passed.
  • PowerShell v2 compatibility checked.

@corbob corbob changed the base branch from develop to release/1.2.0 September 28, 2022 23:46
@coveralls
Copy link

coveralls commented Sep 28, 2022

Coverage Status

Coverage remained the same at 27.582% when pulling 1758e5f on corbob:packages-config-tests into 44c5283 on chocolatey:release/1.2.0.

@corbob corbob changed the title Packages config tests (#886, 2693) Update packages.config install logging and add back resolve or load method for Assembly Resolution Sep 29, 2022
@corbob corbob marked this pull request as ready for review September 29, 2022 16:23
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

LGTM, but need to leave it up to @gep13 to decide whether we should mark the overload as obsolete or not (as already mentioned, I don't mind either way).

@gep13
Copy link
Member

gep13 commented Oct 3, 2022

If everyone is happy, then I would say let's not add the Obsolete attribute, and leave the overload for the method in place.

Update the debug logging when a packages.config file is being used to
output the start and end of the configuration on independent lines. This
allows us to update the test to get these start and end points and not
play guessing games with how long we think the output should be.
Updating this test allows Chocolatey Licensed Extension to be included
and the tests still pass.
Changing the interface or public methods are breaking changes which
we need to wait with implementing before the next major release.
As such, this commit adds back the old way of running the powershell
host by adding an overload with the original contract for the interface
that could possibly be used in other projects that makes use of the
Chocolatey.Lib library.
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13 gep13 merged commit 7eb7620 into chocolatey:release/1.2.0 Oct 6, 2022
@gep13
Copy link
Member

gep13 commented Oct 6, 2022

@corbob thanks for getting this fixed up!

@corbob corbob deleted the packages-config-tests branch October 12, 2022 01:33
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.

None yet

4 participants