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

Enable suppress command in the multitool #2394

Merged
merged 5 commits into from
Oct 12, 2021

Conversation

eddynaka
Copy link
Collaborator

No description provided.

@eddynaka eddynaka marked this pull request as ready for review October 11, 2021 20:01
valid &= options.ExpiryInDays >= 0;
valid &= !string.IsNullOrWhiteSpace(options.Justification);
valid &= (options.Status == SuppressionStatus.Accepted || options.Status == SuppressionStatus.UnderReview);
valid &= DriverUtilities.ReportWhetherOutputFileCanBeCreated(options.OutputFilePath, options.Force, FileSystem);
Copy link

@Bpendragon Bpendragon Oct 11, 2021

Choose a reason for hiding this comment

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

Even if every other item is false, this will return true if the last line is true, is this intended behavior?

Conversely, if every other item is true, this will return false if the last item is false. I can see why that might be useful in this case, but again, is that intended behavior? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I improved the tests.
it will only return true if all are true.

if (timestamps)
{
//suppression.SetProperty("timeUtc", timeUtc.ToString(SarifUtilities.SarifDateTimeFormatMillisecondsPrecision,
// CultureInfo.InvariantCulture));
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Oct 11, 2021

Choose a reason for hiding this comment

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

do we still need this comment #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no we don't.
just removed!
thanks!

{
//suppression.SetProperty("timeUtc", timeUtc.ToString(SarifUtilities.SarifDateTimeFormatMillisecondsPrecision,
// CultureInfo.InvariantCulture));
suppression.SetProperty("timeUtc", timeUtc);
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Oct 11, 2021

Choose a reason for hiding this comment

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

timeUtc

I see the expiryUtc below use a format, if we dont use the same fromat fuction, will the result format differnt #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, they will be printed in the same format.
just improved the code below.

@shaopeng-gh
Copy link
Collaborator

shaopeng-gh commented Oct 11, 2021

un-needed change?


In reply to: 940468657


Refers to: src/Sarif.Multitool.Library/Sarif.Multitool.Library.csproj:66 in 31d2163. [](commit_id = 31d2163, deletion_comment = False)


if (expiryInDays > 0)
{
suppression.SetProperty("expiryUtc", expiryUtc.ToString(SarifUtilities.SarifDateTimeFormatMillisecondsPrecision,
Copy link
Collaborator

@yongyan-gh yongyan-gh Oct 11, 2021

Choose a reason for hiding this comment

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

"expiryUtc"

we can also use nameof() just like setting property for alias #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just updated!
thanks


if (!string.IsNullOrWhiteSpace(alias))
{
suppression.SetProperty(nameof(alias), alias);
Copy link
Collaborator

@yongyan-gh yongyan-gh Oct 11, 2021

Choose a reason for hiding this comment

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

SetProperty

where will these property values be stored? In current spec Suppression doesn't have these properties right? #WontFix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it will be stored in the property bag

[Option(
"alias",
HelpText = "The account name associated with the suppression.")]
public string Alias { get; set; }
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Oct 11, 2021

Choose a reason for hiding this comment

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

Alias

just a thought, outside of Microsoft user may not immediately recognize Alias as Alias for user. do we like to change it to more common like user name, user alias, account name #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good question.
not sure yet the answer.
let's keep this way and we can improve in another iteration.

Copy link
Collaborator

@shaopeng-gh shaopeng-gh left a comment

Choose a reason for hiding this comment

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

:shipit:

}

w.Stop();
Console.WriteLine($"Supress completed in {w.Elapsed}.");
Copy link
Collaborator

@yongyan-gh yongyan-gh Oct 11, 2021

Choose a reason for hiding this comment

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

like the idea to capture the execution time, not all commands implemented this, maybe we can move it to command base to work for all commands? #WontFix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can create an issue to improve this.
won't do this in this PR.
but good suggestion.

@@ -60,6 +61,9 @@ Sarif.Multitool merge C:\Input\*.sarif --recurse --output-directory=C:\Output\ -
: Extract new Results only from New Baseline
Sarif.Multitool query NewBaseline.sarif --expression "BaselineState == 'New'" --output Current.NewResults.sarif

: Suppress Results
Sarif.Multitool suppress current.sarif --justification "some justification" --alias "some alias" --guids --timestamps --expiryInDays 5 --output suppressed.sarif
Copy link
Collaborator

@yongyan-gh yongyan-gh Oct 11, 2021

Choose a reason for hiding this comment

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

suppress

missing a parameter "status" #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added.

@eddynaka eddynaka merged commit 2bdb212 into main Oct 12, 2021
@eddynaka eddynaka deleted the users/ednakamu/enable-suppress-command branch October 12, 2021 12:22
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.

None yet

4 participants