Skip to content

(#2851) fix license validation output #2857

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 5, 2025
Merged

(#2851) fix license validation output #2857

merged 2 commits into from
Jun 5, 2025

Conversation

corbob
Copy link
Member

@corbob corbob commented Oct 14, 2022

Description Of Changes

Update the logic used in LicenseValidation class to correctly detect when regular output is desired and when it's not.

Motivation and Context

When there are certain license conditions, Chocolatey would report things to standard output even if the command issues was requesting to limit output.

Testing

  1. Put a file in the license directory and ran choco list --local-only --limit-output
  2. Then ran choco list --local-only --limitoutput
  3. Then ran choco list --local-only -r
  4. Noted that there is some warnings and errors about missing directories (I followed these as far as I could, at least one is generated by a System provided library, and unlikely that we can suppress)
  5. When the lib directory is there (which it is on a standard Chocolatey install), no errors are output.
  6. Set an empty file as license/chocolatey.license.xml. Noted that limit output prevented the message about the license being invalid

This is currently a draft PR as I intend to add some Pester tests to verify that no extraneous output occurs when limit output is requested.

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 #2851

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
Copy link
Member Author

corbob commented Oct 14, 2022

Question regarding some tests for this: I checked out the add_headers branch I have, and ran the headers test after creating a dummy license file. This then failed any headers tests that expected headers because LicenseValidation was outputting. Are we comfortable with this being a sufficient test for this PR? If we are, then I will rebase this on add_headers and stack these PRs so that this one bases off of that one and then I can update the Pester tests for that one.

2022-10-14_13-55-15

@corbob corbob marked this pull request as ready for review October 31, 2022 18:06
@corbob @AdmiringWorm
(chocolatey#2851) Update LicenseValidation logging logic
7437a22 
The LicenseValidation logging logic was only checking for `-v` or
`--version` as the first arguments. This completely ignored `-r`,
`--limit-output`, and `--limitoutput`. Additionally, a few places
weren't suppressing output when limit output was used. This change
correctly detects if regular output is desired, and if it is then it
marks the output accordingly, otherwise it outputs to the log file only.
@AdmiringWorm AdmiringWorm force-pushed the 2851-fix-license-validation-output branch from 006a2bf to 7437a22 Compare June 3, 2025 13:21
@AdmiringWorm
Copy link
Member

Question regarding some tests for this: I checked out the add_headers branch I have, and ran the headers test after creating a dummy license file. This then failed any headers tests that expected headers because LicenseValidation was outputting. Are we comfortable with this being a sufficient test for this PR? If we are, then I will rebase this on add_headers and stack these PRs so that this one bases off of that one and then I can update the Pester tests for that one.

2022-10-14_13-55-15

I think that should be sufficient enough for these changes. But since this is a while ago, I doubt this test exist anymore.

@AdmiringWorm
(chocolatey#2851) Add pester tests for limit-output flag behavior
6d7200d 
Add comprehensive Pester tests for the `--limitoutput` and related flags
to verify output behavior when license files are missing, misplaced, or
invalid. These tests ensure that no misleading license-related messages
are shown during command execution, improving reliability and user
experience when using the limit-output feature.
@AdmiringWorm AdmiringWorm requested a review from gep13 June 4, 2025 09:24
gep13
gep13 approved these changes Jun 5, 2025
View reviewed changes
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
Copy link
Member

gep13 commented Jun 5, 2025

@corbob thanks for getting this started, and to @AdmiringWorm for getting this over the line!

@gep13 gep13 merged commit 8c6e51a into chocolatey:develop Jun 5, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gep13 gep13 gep13 approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

License Validation outputs some errors and warnings when limit-output enabled.
3 participants
@corbob @AdmiringWorm @gep13