Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding compiler data to AppInsights #404
Adding compiler data to AppInsights #404
Changes from 10 commits
d7d01f9
9a1a8cf
ee25a1c
aa8d558
477a0e9
db3aece
5e6450b
fd254b4
b0f5106
aedb1eb
729453c
9d21968
7ce6027
5112e4a
4fb5fbc
a6a3632
0527ae9
843eeed
2ddf392
ff40b7b
0c7b53b
ed8cb64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Whenever you show an opaque rule id to the user, you should add the readable name. Not everyone can remember what BA4001 is. Also, you've described a fact here 'this data is required to run rule BA4001.ReportPECompilerData' but you haven't described what that data is.
What is this data? Does it make sense that we require a single repo URL for a scan run??
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 changed the helptext. Let me know what do you think.
About your question...I thought about it, and I could not find anything better based on this:
https://docs.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml
my idea was to find one pre-defined variable that any pipeline would be able to get.
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.
same note, what is this data? you don't describe it. #Resolved
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.
updated the helptext, pls, review :)
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 is it ok that some data has gotten dropped? such as the sha256 has? #ByDesign
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.
where I create the CompilerDataLogger, i pass the context. from it I retrieve information such as hash. With that, i can remove from here and just pass the data that i dont have during construction of compilerDataLogger
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 about the hash data? #ByDesign
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.
the hash is constructed with the CompilerDataLogger, so we don't need to worry here.
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.
are we really transforming data that previously existed in the PDB into some other normalized form?
this doesn't seem like a good idea, it seems better to preserve data as it actually exists in the binary.
can you explain the decision? certainly this code needs to be commented if we retain what you've done.
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 were creating a csv and we were using ',' as separator.
with that, I saw cases where the name contained ',' breaking the csv.
this is why i changed from ',' to '_'
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.
typically, what you would do to workaround this would be to encapsulate the data in double quotes. is that an option here?
we really should not be modifying the compiler generated data here. how will you convert it back if you need to unify it again with actual PDB data?
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.
you say in your help comments that this data is required, but it doesn't look like it's required. it's optional.
i think you meant you say that this data is only consumed by the compiler reporting rules, and not relevant otherwise. #Closed
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 changed the relevant code to throw an argumentNullException in case repositoryUri or pipelineName is null/empty.
With that, i cleaned the code
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 are we sending absolute paths? is this right? might this data expose some unwanted information, like a user alias? #Resolved
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.
made a change to transform the localPath to a relativeFilePath base on the targetFileSpecifiers
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.
look at these 'magic constants' where we have special knowledge that backend version is in the first piece.
that tends to be pretty fragile, it might be better for this helper to break out all this data and require the caller to parse into the format.
#Closed
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.
fixed!
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.
Note that it'd be more performant to concatenate all this information into a single string and then pass it to WriteLine.
#Closed
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.
fixed
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 did you create an interface? #Closed
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.
just deleted.
i thought it was going to be useful.