-
Notifications
You must be signed in to change notification settings - Fork 56
Add Include resource via new Import resource kind and resolve operation
#429
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
…ng a `resolve` operation
anmenaga
left a comment
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
| } | ||
| } | ||
| // convert the buffer to a string | ||
| let include_content = match String::from_utf8(buffer) { |
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.
This reminds me that we need to put somewhere in the docs, that input documents for dsc.exe have to be in UTF-8.
This is important for non-English users.
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.
| }; | ||
|
|
||
| // combine the current directory with the Include path | ||
| Ok(Path::new(¤t_directory).join(path)) |
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.
What if the path in the include file is a fully-specified one?
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.
This is already handled on line 127 above
| #[clap(short = 'l', long, help = "Trace level to use", value_enum, default_value = "warning")] | ||
| pub trace_level: TraceLevel, | ||
| #[clap(short = 'l', long, help = "Trace level to use", value_enum)] | ||
| pub trace_level: Option<TraceLevel>, |
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.
please add a comment that default value will be assigned in the code as "warn".
| resources: | ||
| - name: Echo | ||
| type: Test/Echo | ||
| type: test/echo |
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.
Why change to lower case?
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.
this was to explicitly test case-insensitivity and result is case preserving
| for trace_line in stderr.lines() { | ||
| if let Result::Ok(json_obj) = serde_json::from_str::<Value>(trace_line) { | ||
| if let Some(msg) = json_obj.get("Error") { | ||
| error!("Process '{process_name}' id {process_id} : {}", msg.as_str().unwrap_or_default()); |
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.
It would be good to make "Process '{process_name}' id {process_id} part shorter; with it trace lines are too long.
Maybe [<processname>:<procId>]: <text>
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.
We can do that in a follow-up PR along with handling the tracing crate JSON format
PR Summary
Note that this solution is not quite complete yet as it only works for
get. However, through this work, I've realized I need to refactor resourceget,set,testresponses so it is always a list even if just one element. The current separation of a singleton vs list makes it too complicated to complete the work forsetandtest.Microsoft.DSC/Includeresource which includes a file for configuration and also parametersinclude.dsc.yamlshows usageDSC_TRACE_LEVELenv var so that childdscprocesses will emit the same level of traces as I needed it to diagnose problems--trace-levelis specified, it will replace any existingDSC_TRACE_LEVELenv var with the new value,dscnow uses this env var unless--trace-levelis specifiedwarninglevel towarnto match the code to keep it simpleImportresource kind that needs to support aresolveoperation and return aResolveResultresolve, I've also madegetoptional since it doesn't make sense for anImportresourceconfiguratoris created to perform the operation against the resolved contentfind_resources()works by internally handling case-insensitivity for searching. This removed a bunch of code that made things lowercase before calling this function.parse_input_to_json()that will accept JSON or YAML and emit JSON since the file pointed to byIncludemay either be JSON or YAML for the configuration and parameters filePR Context
Fix #412