Skip to content

Add support for the scanning API #74

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

Merged
merged 5 commits into from
Feb 11, 2019
Merged

Add support for the scanning API #74

merged 5 commits into from
Feb 11, 2019

Conversation

meskio
Copy link
Contributor

@meskio meskio commented Jan 30, 2019

I have created a new class SdScanningClient that exposes most of the secure scanning API. I thought it makes sense to be it's own class, as SdSecureClient is already pretty big and there are things like policies that exists both in the scanning part and in the secure part. If we want everything in SdSecureClient I can explore mixins or some inheritage tricks to have all the methods in one while the code is split in several files or just have one big class.

I have imported some files from anchore-cli into sdcclient/_anchore to bring all the API calls. At some point it might make sense to refactor all that out into the SdScanningClient, if you consider that a blocker to merge this pull-req I'm happy to work on that. (I've cleaned it up, there is no anchore code anymore)

I have refactor SdMonitorClient and SdSecureClient in it's own files and clean up some pep8 errors, the _client.py file was getting too big.

Copy link
Contributor

@figarocorso figarocorso left a comment

Choose a reason for hiding this comment

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

Terrific work 👏 !

As your code was looking really nice, I just pointed a bunch of cosmetic changes so we can make it more legible at some places (specially when dealing with url building, if-elif-else clauses or suggesting small refactors).

The code as it is looks OK, so feel free to accept all of them, some or none. Nice having you around this repo 👍

@meskio
Copy link
Contributor Author

meskio commented Jan 31, 2019

I think I have fixed all your comments.

_client.py has grown too big. Moving SdcMonitor and SdcSecure into
separated files.
Make the code pep8 compilant except for line lenght (E501) and for
comparisons to none (E711) and bool (E712) using '==' or '!='.
Add image, registries and subscription support. Importing some anchore
helper functions from anchore-cli into sdcclient/_anchore.
@omerazr omerazr self-requested a review February 9, 2019 01:34
@omerazr omerazr self-assigned this Feb 9, 2019
@omerazr
Copy link
Contributor

omerazr commented Feb 9, 2019

Travis is failing

@davideschiera
Copy link
Contributor

@omer-sd the failure might not be related to the changes in the PR. I'll try to manually restart the build.

@meskio
Copy link
Contributor Author

meskio commented Feb 11, 2019

The travis CI error says PYTHON_SDC_TEST_ACCESS_KEY: unbound variable, it looks like the API somehow didn't get set on this run.

@davideschiera any luck restarting the build manually?

@davideschiera
Copy link
Contributor

@meskio The restarting part was easy, but unfortunately the build is still failing, and it's a configuration issue, nothing related to your changes. I started taking a look at the Travis configuration, but didn't find anything useful yet.

If @omer-sd is ok with this, an idea could be merging the PR anyway so that your work goes mainstream (the build should work as expected on dev/master branches), and in the meantime keep troubleshooting the build issue. @omer-sd please let me know. Thanks!

@omerazr
Copy link
Contributor

omerazr commented Feb 11, 2019

This is approved. We can merge the code. We will handle the Travis failure internally.

@davideschiera davideschiera merged commit 6170bdf into sysdiglabs:master Feb 11, 2019
@mstemm mstemm mentioned this pull request Feb 14, 2019
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.

4 participants