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.

Sign up for GitHub

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

Jump to bottom

Feat: add python to auto detect for --all-projects #964

Merged
merged 1 commit into from
Jan 29, 2020
Merged

Feat: add python to auto detect for --all-projects #964

merged 1 commit into from
Jan 29, 2020

Conversation

aviadhahami
Copy link
Contributor

@aviadhahami aviadhahami commented Jan 22, 2020 •
edited

  • Ready for review
  • Reviewed by Snyk internal team

What does this PR do?

Adding Pipfile and requirements.txt to auto-detection w/ --all-projects

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

BST-1112

@aviadhahami aviadhahami requested a review from a team as a code owner January 22, 2020 14:53
@aviadhahami aviadhahami self-assigned this Jan 22, 2020
@ghost ghost requested review from dkontorovskyy and gitphill January 22, 2020 14:53
gitphill
gitphill reviewed Jan 22, 2020
View reviewed changes
src/lib/find-files.ts Outdated Show resolved Hide resolved
gitphill
gitphill reviewed Jan 22, 2020
View reviewed changes
Pipfile Outdated Show resolved Hide resolved
@lili2311 lili2311 changed the title Feat/add python to auto detect Feat: add python to auto detect for --all-projects Jan 23, 2020
@aviadhahami aviadhahami force-pushed the feat/add-python-to-auto-detect branch 3 times, most recently from f6f0c6c to 4fe746c Compare January 23, 2020 13:16
orsagie
orsagie reviewed Jan 23, 2020
View reviewed changes
test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts
@@ -145,8 +145,8 @@ export const AllProjectsTests: AcceptanceTests = {
allProjects: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add skipUnresolved: true here so we don't fail on python plugin

lili2311
lili2311 reviewed Jan 24, 2020
View reviewed changes
src/lib/detect.ts
@@ -41,6 +41,8 @@ export const AUTO_DETECTABLE_FILES: string[] = [
'packages.config',
'project.json',
'project.assets.json',
'Pipfile',
'requirements.txt',
Copy link
Contributor

@lili2311 lili2311 Jan 24, 2020 •
edited

Choose a reason for hiding this comment

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

is there a test for requirements.txt? not sure we really need what do you think?

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 atm as Pip doesnt pass

@orsagie orsagie force-pushed the feat/add-python-to-auto-detect branch 8 times, most recently from a9ab75d to 4fda770 Compare January 27, 2020 12:09
aviadhahami
aviadhahami commented Jan 27, 2020
View reviewed changes
test/acceptance/cli-test/cli-test.all-projects.spec.ts Outdated Show resolved Hide resolved
test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts Outdated
// Python plugin (actually "custom-auto-detect" plugin) doesn't return
// "targetFile" prop as expected, so we picked it from another part of
// the req;
const {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a part of the hack we did in order to push the relevant target file
Should be removed once the pipfile issue is resolved

test/acceptance/cli-test/cli-test.all-projects.spec.ts Outdated
// Forcing targetFile into the payload as the new plugin doesn't return
// it;
t.same(
{...pipAllProjectsBody}, // targetFile: pipAllProjectsBody.displayTargetFile},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hack like in test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts#230

test/acceptance/cli-test/cli-test.all-projects.spec.ts Outdated
// Forcing targetFile into the payload as the new plugin doesn't return
// it;
t.same(
{ ...pipAllProjectsBody }, // targetFile: pipAllProjectsBody.displayTargetFile},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hack like test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts#L230

src/lib/detect.ts
@@ -41,6 +41,8 @@ export const AUTO_DETECTABLE_FILES: string[] = [
'packages.config',
'project.json',
'project.assets.json',
'Pipfile',
'requirements.txt',
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 atm as Pip doesnt pass

test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts Outdated
});
t.ok(spyPlugin.withArgs('rubygems').calledOnce, 'calls rubygems plugin');
t.ok(spyPlugin.withArgs('npm').calledOnce, 'calls npm plugin');
t.ok(spyPlugin.withArgs('maven').calledOnce, 'calls maven plugin');
t.ok(spyPlugin.withArgs('pip').calledOnce, 'calls maven plugin');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

text need to be changed to pip plugin

@orsagie orsagie force-pushed the feat/add-python-to-auto-detect branch from 25ca6ea to 66dc964 Compare January 27, 2020 16:28
@dkontorovskyy dkontorovskyy force-pushed the feat/add-python-to-auto-detect branch 2 times, most recently from d885688 to 7d7e02f Compare January 28, 2020 08:56
@dkontorovskyy dkontorovskyy force-pushed the feat/add-python-to-auto-detect branch 8 times, most recently from 33965e1 to aff0972 Compare January 28, 2020 10:07
gitphill
gitphill reviewed Jan 28, 2020
View reviewed changes
test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts Outdated
@@ -9,7 +9,7 @@ interface AcceptanceTests {
}

export const AllProjectsTests: AcceptanceTests = {
language: 'Mixed (Ruby & Npm & Maven)',
language: 'Mixed (Ruby & Npm & Maven & Pip)',
Copy link
Contributor

Choose a reason for hiding this comment

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

have just made this 'Mixed' on my go PR

@dkontorovskyy dkontorovskyy force-pushed the feat/add-python-to-auto-detect branch 4 times, most recently from a2d9682 to c004843 Compare January 28, 2020 12:28
@gitphill gitphill force-pushed the feat/add-python-to-auto-detect branch from c004843 to 8e2702c Compare January 28, 2020 14:05
dkontorovskyy
dkontorovskyy approved these changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@dkontorovskyy dkontorovskyy left a comment •
edited

Choose a reason for hiding this comment

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

LGTM
gif

@gitphill gitphill force-pushed the feat/add-python-to-auto-detect branch 3 times, most recently from 0291fb5 to c2ffa9b Compare January 29, 2020 13:04
@gitphill gitphill force-pushed the feat/add-python-to-auto-detect branch from c2ffa9b to 36d59cc Compare January 29, 2020 13:07
@dkontorovskyy
Copy link
Contributor

Not really sure if paket fixtures refactor should be in here

@gitphill gitphill force-pushed the feat/add-python-to-auto-detect branch 2 times, most recently from acd0522 to bcb37cd Compare January 29, 2020 13:44
@lili2311 @gitphill
feat: add python to auto manifest discovery
e1651dd 
Adding support for Pipfile and requirements.txt with --all-projects.
Refactoring tests as well as adding tests for Pipfile and requirements.txt.
@gitphill gitphill force-pushed the feat/add-python-to-auto-detect branch from bcb37cd to e1651dd Compare January 29, 2020 14:04
@gitphill gitphill merged commit e08bc8f into master Jan 29, 2020
@gitphill gitphill deleted the feat/add-python-to-auto-detect branch January 29, 2020 15:10
@snyksec
Copy link

snyksec commented Jan 29, 2020

🎉 This PR is included in version 1.287.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lili2311 lili2311 lili2311 left review comments

@gitphill gitphill gitphill left review comments

@dkontorovskyy dkontorovskyy dkontorovskyy approved these changes

@orsagie orsagie orsagie left review comments

Assignees

@aviadhahami aviadhahami

Labels
released
Projects
None yet
Milestone
No milestone
6 participants
@aviadhahami @dkontorovskyy @snyksec @lili2311 @gitphill @orsagie