-
Notifications
You must be signed in to change notification settings - Fork 636
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
Add support for v7.x CER Component SDK #14501
Conversation
|
||
foreach (var file in filesToSend) | ||
{ | ||
cerDLL.SetMultiStringParam(ReportMultiStringParamKey.MultiStringKeyExtraFile, file, filesToSend.Count + 1); |
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.
will SetMultiStringParam
actually send the file content ?
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 filesToSend.Count + 1 ?
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 set the filenames that later is packed up by CER (Can be seen happening in the logs).
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.
I can maybe remove +1. Let me check.
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.
Yes, should be Count.
@sm6srw did you test this out ? There is a debug mode to cause crashes on node placement (if I remember correctly) |
Yes, I have tested it. I have checked the various logs and the result on the CER site. |
@@ -187,78 +187,75 @@ internal static bool ShowCrashErrorReportWindow(DynamoViewModel viewModel, Crash | |||
var cerDir = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), "DynamoCER_Report_" + | |||
DateTime.Now.ToUniversalTime().ToString("yyyy-MM-dd-HH-mm-ss"))); | |||
|
|||
using (Scheduler.Disposable.Create(() => { |
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.
@pinzart90 I had to remove this. We have no control anymore on when these files are consumed. I have seen them missing in the report in about half the runs I have attempted in my testing and that confused me for a while. Regardless, I don't think we need it. Windows 10 and later has a "Storage Sense" service that removed these temp files if they have not been touched for 30 days.
var upiConfigFilePath = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "upiconfig.xml"); | ||
foreach (var file in filesToSend) | ||
{ | ||
cerDLL.SetMultiStringParam(ReportMultiStringParamKey.MultiStringKeyExtraFile, file, filesToSend.Count); |
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.
Does this function call actually send out the files or just the file paths ?
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.
OK..I saw the file content on some of the reports
Purpose
This pull request add support for the latest CER tool.
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Add support for v7.x CER Component SDK
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of