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

Changed List operation errors to non-terminating warnings #135

Merged
merged 9 commits into from
Aug 4, 2023

Conversation

anmenaga
Copy link
Collaborator

@anmenaga anmenaga commented Aug 4, 2023

PR Summary

This fixes List part of #121 :
if part of dsc resource list, we can emit a warning type message to stderr but continue discovery listing the results

Example output after the fix:

PS C:\DSCv3> dsc.exe resource list
Warning -> Test/TestProvider Could not start pwshZZZ: program not found
Warning -> DSC/PowerShellGroup Failed to parse resource: a-{"type":"PowerShellGet/MSFT_PSModule",...,"requires":"DSC/PowerShellGroup"} -> expected value at line 1 column 1
Warning -> PowerShellGet/MSFT_PSRepository Provider source 'DSC/PowerShellGroup' missing 'requires' property for resource 'PowerShellGet/MSFT_PSRepository'

Type                        Version  Requires        Description
----------------------------------------------------------------------------------------------------------------------------
Microsoft.Windows/Registry  0.1.0                    Registry configuration provider for the Windows Registry
DSC/Group                   0.1.0                    All resources in the supplied configuration is treated as a group.
Test/TestGroup              0.1.0
DSC/AssertionGroup          0.1.0                    `test` will be invoked for all resources in the supplied configuration.
DSC/ParallelGroup           0.1.0                    All resources in the supplied configuration run concurrently.
DSC/PowerShellGroup         0.1.0                    Resource provider to classic DSC Powershell resources.
Test/TestResource1          1.0.0    Test/TestGroup  This is a test resource.
Microsoft/OSInfo            0.1.0                    Returns information about the operating system.
Test/TestResource2          1.0.1    Test/TestGroup  This is a test resource.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

We should discuss the fundamental design so that it's more general and not specific to command discovery init

@@ -15,6 +15,7 @@ use std::path::Path;
pub struct CommandDiscovery {
pub resources: BTreeMap<String, DscResource>,
provider_resources: Vec<String>,
initialization_messages: Vec<StreamMessage>,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be a member of this struct. I was imaging we'd have a new log struct that everything calls and would handle output to STDERR and also if it's emitted as JSON or text. This would replace all direct calls to eprint

Copy link
Collaborator Author

@anmenaga anmenaga Aug 4, 2023

Choose a reason for hiding this comment

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

yes, new log struct that everything calls and would handle output to STDERR and also if it's emitted as JSON or text - that is on todo list. Current eprintln calls will later be refactored to calling that new API.

Copy link
Collaborator Author

@anmenaga anmenaga Aug 4, 2023

Choose a reason for hiding this comment

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

the problem with calling logging functions (eprintln's fow now; new 'log' API later) right as messages are generated is that we change message types depending on what higher level operation is executing. Which means this logic has to be somehow passed to the 'log', which complicates things.
In any way, these 2 are just different implementation approaches that we can switch between later if we won't like this one; the user will have the same UX in both cases.

@SteveL-MSFT SteveL-MSFT merged commit 4817663 into main Aug 4, 2023
4 checks passed
@SteveL-MSFT SteveL-MSFT deleted the errors_warnings branch August 4, 2023 21:32
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.

2 participants