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

Track rubygems projects' targetFilerelativePath #975

Closed

Conversation

aviadhahami
Copy link
Contributor

  • Ready for review

What does this PR do?

Added analytics logging for rubygems projects using --file flag;

@aviadhahami aviadhahami requested a review from a team as a code owner January 26, 2020 15:38
@ghost ghost requested review from dkontorovskyy and lili2311 January 26, 2020 15:38
@snyksec
Copy link

snyksec commented Jan 26, 2020

Messages
📖

This PR will not trigger a new version. It doesn't include any commit message with feat or fix.

Generated by 🚫 dangerJS against bfcf617

@aviadhahami aviadhahami force-pushed the logging/track-ruby-project-relative-paths branch from 09e881a to bfcf617 Compare January 26, 2020 15:45
for (let i = 0; i < resArray.length; i++) {
results.push(_.assign(resArray[i], { path }));

if (options.file && resArray[i].packageManager === 'rubygems') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be better done
after https://github.com/snyk/snyk/blob/a724b210cd2ad38d7124ff6257ed571ee34d973a/src/lib/snyk-test/run-test.ts#L353 as there might be multiple files that are tested (--all-projects).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean

@@ -155,6 +155,12 @@ async function monitor(...args0: MethodArgs): Promise<any> {
const targetFileRelativePath = tFile
? pathUtil.join(pathUtil.resolve(path), tFile)
: '';

// target file logging as a port of [Jira: BST-1124]
if (packageManager === 'rubygems' && options.file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should log this even if the options.file option was not set

Copy link
Contributor

Choose a reason for hiding this comment

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

so we definitely don't already track this in existing analytics?

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't

Copy link
Contributor

Choose a reason for hiding this comment

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

what about older CLIs? Have you considered doing this in the main app instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lili2311 you are right, on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lili2311 in registry I can only get the file (flag usage and file itself), but not targetFileRelativeFile, do we still want to proceed w/ registry?

@aviadhahami
Copy link
Contributor Author

ok, IMO this can be dropped in favor of https://github.com/snyk/registry/pull/11380

@lili2311 wdyt?

@lili2311
Copy link
Contributor

@aviadatsnyk yep agreed it would be better there

@aviadatsnyk
Copy link
Contributor

@aviadhahami ^^

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.

5 participants