-
Notifications
You must be signed in to change notification settings - Fork 564
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
chore: change project name source #3570
Conversation
4411efa
to
e2539a6
Compare
e2539a6
to
8ef422d
Compare
@@ -32,6 +32,11 @@ export interface SnykIacTestOutput { | |||
export interface Results { | |||
resources?: Resource[]; | |||
vulnerabilities?: Vulnerability[]; | |||
metadata?: Metadata; |
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.
Thought: Does the generation of the metadata depends on the existence of valid results?
If there are no results would there not be metadata?
Unless there's a direct coupling between the existence of results
and metadata
it makes more sense to me that metadata
would be a top-level property of TestOutput
.
If the project name generation is conditional, would it make sense to have the metadata
property as required, and projectName
within it as conditional?
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 the generation of the metadata depends on the existence of valid results?
Currently, it does because we are generating the metadata in the results processing stage which we enter only when there are valid results.
AFAIK when we don't generate results we are only displaying an error without any information about the tests so there is no reason to generate the metadata.
If the project name generation is conditional, would it make sense to have the metadata property as required, and projectName within it as conditional?
That's an implementation detail that we can always change in the future if necessary.
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 don't think it's an implementation detail, as it actually defines the contract we have with snyk-iac-test
in the different flows and user stories.
On the other hand, the fact that we calculate the project name as part of the results processing step is an implementation detail, as it can be generated in another scope which then allows us to return it in all the desired flows.
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.
as it actually defines the contract we have with snyk-iac-test in the different flows and user stories
But the current flows and user stories do not display the project name if the test didn't produce results, this is why I think it is an implementation detail (similar to what you said in your 2nd half of the message).
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.
In addition to that, personally, I believe we should stop displaying the project name in the test output because it is misleading because it doesn't match the platform definition of a "project".
orgSettings: IacOrgSettings; | ||
options: any; | ||
}) { | ||
const projectName = |
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.
Suggestion: I think this default value can cause more confusion than good.
If we considered projectName
to be required and assert it with this default value, wouldn't it make more sense to make it required when receiving it from snyk-iac-test
?
In what scenarios does snyk-iac-test
not return a project name and we require this default value?
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.
If we considered projectName to be required and assert it with this default value, wouldn't it make more sense to make it required when receiving it from snyk-iac-test?
I agree with you that it would make sense if we would consider it required but currently, we do not.
as I said above it is an implementation detail so if in the future we see it as problematic we can always change it.
In what scenarios does snyk-iac-test not return a project name and we require this default value?
When we failed to generate results.
(BTW I might be wrong here but I'm pretty sure that if the PE failed to return results we are not even displaying the project name)
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.
In that case at the very least if results
exists that means metadata
should exist as well, meaning there'd be no need to have it as optional if it's there
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 reason that metadata
is optional is the same reason that resources
is optional.
I agree with your logic here but I rather align with the current results
type structure.
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.
As mentioned in the comments it'd make more sense to me personally to have metadata
as a top-level required property but I don't consider it a blocker and we can always re-iterate on this in the future.
Apart from that, LGTM ✅
What does this PR do?
Removes the project name calculation that we are doing in the CLI and instead starts using the project name that we are returning from
snyk-iac-test