-
Notifications
You must be signed in to change notification settings - Fork 920
(#2591) Add headers to limit output commands #2856
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
Conversation
The tests added here failed on the pin output with a header due to it being overloaded I think... It might require some further investigation, and perhaps we should do that before merging this in? For completeness this is the Test Kitchen output:
|
Converting this back to draft as it requires quite a bit of rework and validation. |
@corbob I may have taken this for a quick spin, and rebased on the head of the develop branch. Just wanted to give you a heads up, so that you weren't surprised. |
src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs
Outdated
Show resolved
Hide resolved
@corbob I have taken the liberty of rebasing this PR onto the head of the develop branch. |
I have a number of local changes on my machine that I will push up here when I get a chance. This includes the outcomes from a chat with @pauby and a few additional changes after doing some testing of things. |
During a discussion with @pauby the decision was made to:
|
ad2cad9
to
af94e3b
Compare
@AdmiringWorm I have run the Test-Kitchen tests for this PR, and they have all passed. |
@AdmiringWorm this is now ready for a review. Thanks |
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.
Some minor nitpicks.
src/chocolatey/infrastructure.app/services/ChocolateyConfigSettingsService.cs
Show resolved
Hide resolved
src/chocolatey/infrastructure.app/services/ChocolateyConfigSettingsService.cs
Show resolved
Hide resolved
When a user asks for limited output from Chocolatey, it is not uncommon to pipe that output to `ConvertFrom-String` or `ConvertFrom-Csv` and manually add headers to get back an object. This allows for getting a header row back so that the end user doesn't need to add their own headers and discern what they are. This also adds a StringResources static class that allows us to store constant strings in and use them across the code to reduce duplication. A new OutputHelpers class has been introduced in an attempt to simplify the concatenation of the limited output, when a collection of strings are joined to the "|" character. Some new Pester tests have also been added to verify the output from the commands, both when the new '--include-headers' option is used, as well as the 'alwaysIncludeHeaders' feature.
We found an issue when trying to pass multiple arguments through to Invoke-Choco. After a few different attempts to get things working, we settled on explicitly creating an array of arguments, which eant a minor change in Invoke-Choco itself to handle this correctly. The logic for skipping the `choco license` test was simplified to only include that command _when_ licensed is in play, rather than trying to skip that test when license is in play. Also added the required setup to allow execution of the required commands, i.e. install an older version of a package to allow `choco outdated` to work, and setting an apikey so the headers appear when doing `choco apikey`. Took the opportunity to fix a spelling mistake that was in nother file as well: encryped -> encrypted
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
Description Of Changes
When a user asks for limited output from Chocolatey, it is not uncommon to pipe that output to
ConvertFrom-String
orConvertFrom-Csv
and manually add headers to get back an object. This allows for getting a header row back so that the end user doesn't need to add their own headers and discern what they are.Adds a new feature that allows the user to always display the headers if desired. This defaults to off so that we don't break any existing scripts.
Motivation and Context
Reduce the amount of work end users need to do in order to consume limited output.
Testing
Ran through the tests in the Vagrant test environment. This resulted in two failures that appear unrelated to my changes. I will investigate those further before marking this PR as ready.
NOTE: This will require Chocolatey Licensed Extension changes.
Change Types Made
Related Issue
Fixes #2591
Change Checklist