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/windows tests in cicd pipeline #244

Merged
merged 6 commits into from
Aug 17, 2020
Merged

Feat/windows tests in cicd pipeline #244

merged 6 commits into from
Aug 17, 2020

Conversation

ivanstanev
Copy link
Contributor

@ivanstanev ivanstanev commented Aug 7, 2020 •
edited by agatakrajewska
Loading

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Adds support for Windows in the plugin and adjusts the tests and CI pipeline accordingly.

Where should the reviewer start?

Commit by commit 😉

What are the relevant tickets?

Jira ticket RUN-1100

@ivanstanev ivanstanev self-assigned this Aug 7, 2020
@ivanstanev ivanstanev force-pushed the chore/windows-tests-in-cicd-pipeline branch from fb3573b to 99a03b7 Compare August 7, 2020 14:20
@agatakrajewska agatakrajewska force-pushed the chore/windows-tests-in-cicd-pipeline branch 21 times, most recently from f9c8d91 to 0d26de4 Compare August 14, 2020 16:25
ivanstanev
ivanstanev commented Aug 14, 2020
View reviewed changes
Copy link
Contributor Author

@ivanstanev ivanstanev left a comment

Choose a reason for hiding this comment

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

Nice!

@agatakrajewska agatakrajewska marked this pull request as ready for review August 14, 2020 17:04
@agatakrajewska agatakrajewska requested a review from a team as a code owner August 14, 2020 17:04
@agatakrajewska agatakrajewska self-assigned this Aug 14, 2020
@agatakrajewska agatakrajewska force-pushed the chore/windows-tests-in-cicd-pipeline branch 9 times, most recently from 2090272 to 9dd589a Compare August 15, 2020 09:47
shaimendel
shaimendel reviewed Aug 16, 2020
View reviewed changes
Copy link
Contributor

@shaimendel shaimendel left a comment

Choose a reason for hiding this comment

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

first - this is absolutely amazing!! 🤯

  • since one of the commits is a feat maybe we want to rename the PR title to be feat as well?
  • regarding the docker socket that's not covered in tests, is it working at all? or just not covered

package.json Outdated
@@ -17,6 +17,7 @@
"lint": "prettier --check '{lib,test}/**/*.ts' && tslint --format stylish '{lib,test}/**/*.ts'",
"format": "prettier --loglevel warn --write '{lib,test}/**/*.ts' && tslint --fix --format stylish '{lib,test}/**/*.ts'",
"test": "npm run lint && npm run unit-test",
"test-windows": "prettier --check \"{lib,test}/**/*.ts\" && tap test/windows -R=spec --timeout=300",
Copy link
Contributor

Choose a reason for hiding this comment

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

should the prettier part be in here?

Copy link
Contributor

Choose a reason for hiding this comment

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

well, actually I can just npm run lint instead 👍

lib/image-type.ts
@@ -1,3 +1,4 @@
import { normalize as normalizePath } from "path";
Copy link
Contributor

Choose a reason for hiding this comment

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

we do we sometimes use normalize as is and sometimes rename is to normalizePath?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will unify that as use just normalizePath as it's more explicit! 👍

test/windows/application-scans.test.ts
@@ -0,0 +1,81 @@
import * as path from "path";
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference in the windows specific tests? why do we need them?

Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't much difference, just normalising the paths, etc. We've decided to go this way, as it wasn't feasible to run all the tests on windows, lots of them are irrelevant and/or we didn't quite work out why they are failing (like the docker socket one). So the best solution for now was to have few dedicated tests, covering high level paths. So there's duplication but I think it's nicer than having a check if platform is windows in every test, wdyt?

@agatakrajewska agatakrajewska force-pushed the chore/windows-tests-in-cicd-pipeline branch from 9dd589a to fe3b9ff Compare August 17, 2020 08:18
@agatakrajewska agatakrajewska changed the title Chore/windows tests in cicd pipeline Feat/windows tests in cicd pipeline Aug 17, 2020
@agatakrajewska agatakrajewska force-pushed the chore/windows-tests-in-cicd-pipeline branch from fe3b9ff to 522bad9 Compare August 17, 2020 10:58
@agatakrajewska agatakrajewska force-pushed the chore/windows-tests-in-cicd-pipeline branch from 84aad3d to 93e135b Compare August 17, 2020 12:36
ivanstanev and others added 5 commits August 17, 2020 14:06
@ivanstanev @agatakrajewska
feat: normalize all paths used to support scanning on Windows
e9b9ccb 
The plugin currently works only with Unix/POSIX-style paths and cannot scan container images while running on Windows.
@agatakrajewska
feat: Added support for docker socket on win
6c271ad
@ivanstanev @agatakrajewska
fix: trim parsed apk output to get rid of Windows newlines
47d206e
@agatakrajewska
chore: added windows specific tests
ec7cf62 
Added mainly high level tests covering plugin response and a unit test covering pulling from container registry.
@agatakrajewska
chore: added extra coverage for oci-archive
24fe47c
@agatakrajewska agatakrajewska force-pushed the chore/windows-tests-in-cicd-pipeline branch from 93e135b to 24fe47c Compare August 17, 2020 13:06
@github-actions
Copy link

github-actions bot commented Aug 17, 2020 •
edited by agatakrajewska
Loading

Expected release notes (by @agatakrajewska)

features:
Added support for docker socket on win (6c271ad)
normalize all paths used to support scanning on Windows (e9b9ccb)

fixes:
trim parsed apk output to get rid of Windows newlines (47d206e)

others (will not be included in Semantic-Release notes):
added extra coverage for oci-archive (24fe47c)
added windows specific tests (ec7cf62)
run Windows builds on CircleCI (91ee5b6)
ignore node diagnostic report (b0e2eba)
enforce release after tests, lint and build (467de68)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@agatakrajewska agatakrajewska merged commit 3b14314 into master Aug 17, 2020
@agatakrajewska agatakrajewska deleted the chore/windows-tests-in-cicd-pipeline branch August 17, 2020 14:38
@snyksec
Copy link

snyksec commented Aug 17, 2020

🎉 This PR is included in version 3.17.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ivanstanev ivanstanev linked an issue Sep 29, 2020 that may be closed by this pull request
The plugin cannot handle relative paths in container tar files in some cases #218
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agatakrajewska agatakrajewska agatakrajewska left review comments

@shaimendel shaimendel shaimendel approved these changes

Assignees

@agatakrajewska agatakrajewska

Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The plugin cannot handle relative paths in container tar files in some cases
4 participants
@ivanstanev @snyksec @agatakrajewska @shaimendel