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

fix: add targetRuntime meta information to monitor #4916

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

dotkas
Copy link
Contributor

@dotkas dotkas commented Oct 24, 2023

The entire foundation for utilizing the targetRuntime exists in the Snyk backend, but is not utilized by snyk monitor.

Adding the targetRuntime will decorate the snyk monitor results with the Target Runtime information, used in multiple language ecosystems.

Here an example from .NET, before:
Screenshot 2023-10-24 at 21 58 53

After:
Screenshot 2023-10-24 at 21 58 56

This will also pave the way for support of snyk monitor of a single project with multiple target runtimes attached, without overwriting the same project multiple times.

@dotkas dotkas requested review from a team as code owners October 24, 2023 19:59
@dotkas dotkas force-pushed the dotkas/add-meta-information-to-test-result branch from e6ba7fc to 4836057 Compare October 24, 2023 20:00
@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2023

Warnings
⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones.
A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax.
Files found:

  • test/tap/cli-monitor.acceptance.test.ts

Generated by 🚫 dangerJS against 6a92147

@dotkas dotkas enabled auto-merge (squash) October 25, 2023 09:38
Copy link
Contributor

@darscan darscan left a comment

Choose a reason for hiding this comment

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

Blocking. We need to verify that this will not break existing pipelines (i.e. that tests map to the same projects that they used to, and that this won't create a flood of new projects in the system)

@dotkas
Copy link
Contributor Author

dotkas commented Oct 25, 2023

@darscan I already did that.

@darscan
Copy link
Contributor

darscan commented Oct 25, 2023

So, you've manually tested every combination of cli-plugin and input argument, for every ecosystem against existing projects in the system?

@dotkas
Copy link
Contributor Author

dotkas commented Oct 25, 2023

@darscan I checked which CLI plugin would populate meta.targetRuntime, could not find anything other than snyk-nuget-plugin that is used by the CLI currently. What have I missed?

@dotkas
Copy link
Contributor Author

dotkas commented Oct 25, 2023

@darscan it only applies to runs done with the beta-flag --dotnet-runtime-resolution, meaning existing .NET projects are not affected.

@darscan
Copy link
Contributor

darscan commented Oct 25, 2023

Thanks, that clears things up a lot 👍

@dotkas
Copy link
Contributor Author

dotkas commented Oct 25, 2023

Disabling auto-merge and double-checking everything. @darscan made me paranoid.

@darscan
Copy link
Contributor

darscan commented Oct 25, 2023

it only applies to runs done with the beta-flag --dotnet-runtime-resolution, meaning existing .NET projects are not affected.

But what happens when that feature goes GA?

@dotkas
Copy link
Contributor Author

dotkas commented Oct 25, 2023

Continuing the discussion outside this PR.

@dotkas dotkas merged commit 068b111 into master Oct 25, 2023
12 checks passed
@dotkas dotkas deleted the dotkas/add-meta-information-to-test-result branch October 25, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants