search

fabric8-analytics / fabric8-analytics-license-analysis

License Analysis
GNU General Public License v3.0
6 stars 25 forks source link

Separate API end point for license service #80

Closed sunilk747 closed 6 years ago

sunilk747 commented 6 years ago

Separate API endpont for license service with auth implementation.

miteshvp commented 6 years ago

@tisnik - I figured out that importing pycrypto indirectly via conftest.py results in failure since gcc is needed to build pycrypto and we do not have one in runtests.sh. Can you tell me why is the code coverage not measured for other tests if rest_api is written differently? And can you please help here based on your experience with previous repos? I will revert the commits since then.

miteshvp commented 6 years ago

And we are back to normal. @tisnik it will be really helpful if you could provide some insights into this test cases failure. cc @sunilk747

msrb commented 6 years ago

Separate API endpont for license service with auth implementation.

If the intention is to make this service publicly available for OSIO users, and thus authentication is required, then I agree with @humaton that we should move the auth code to a common library and use the library in both API server and here. Authentication is a sensitive business and there is no room for mistakes. Better keep the code in one place.

tisnik commented 6 years ago

Sure @miteshvp.

If use the flask client in test_rest_api.py, then all the code (tests+license analysis sources) are actually run in one Python virtual machine. It means that:

1) pycov is able to check what statements are 'hits' by tests 2) we can use mocks where appropriate 3) all exceptions etc. are displayed on CI and usually catch by tests as well (no false positives)

In the second approach, ie. when the service runs separately, the coverage is not measured at all and mocks wont work.

But actually I see your point: you want to test the service as is (ie as black box). This makes perfect sense, but please let's do it differently. Let's create new script called run_integration_tests.sh that will run only selected (properly tagged) tests that really check the REST API. You can then copy your variant of test_rest_api.py into new module and call the API, check the response object etc. etc.

I'll gladly help you with that setup. WDYT?

miteshvp commented 6 years ago

@tisnik - Thanks for the detailed explanation. It was really useful. And I totally like your idea of separating unit-tests with integration-tests. @sunilk747 can you please take this forward.

sara-02 commented 6 years ago

[test]

tisnik commented 6 years ago

@sara-02 could you please copy&paste log here? @all do we really going to support Python 2?

jpopelka commented 6 years ago

Any idea why that can be so?

I don't think making it Python 2 compatible is worth the effort ;-) https://pythonclock.org

abs51295 commented 6 years ago

@tisnik we are fixing this in #83 . Please wait for few minutes. We are very close as it seems :)

rootAvish commented 6 years ago

This issue is not related to Py-2, Py-3.

rootAvish commented 6 years ago

Please check #83

centos-ci commented 6 years ago

@sunilk747 Your image is available in the registry: docker pull registry.devshift.net/fabric8-analytics/license-analysis:SNAPSHOT-PR-80

centos-ci commented 6 years ago

@sunilk747 Your image is available in the registry: docker pull registry.devshift.net/fabric8-analytics/license-analysis:SNAPSHOT-PR-80

centos-ci commented 6 years ago

@sunilk747 Your image is available in the registry: docker pull registry.devshift.net/fabric8-analytics/license-analysis:SNAPSHOT-PR-80

tisnik commented 6 years ago

@ravsa this should not be merged:

1) @humaton requested changes 2) there are no tests for the new code (and the code coverage drops far below the threshold!!!) 3) there are no documentation

2+3 -> this PR won't pass DoD

miteshvp commented 6 years ago

Thanks Pavel for your comments, btw, what is DoD? @sunilk747 - Please make sure that we include tests for the new functionality and improve on the test coverage. @humaton - I see you created https://github.com/openshiftio/openshift.io/issues/2676. I will ask Sunil to create a new PR to accommodate common library changes. If you're ok, and once we increase the test coverage, we can let it merge. WDYT?

tisnik commented 6 years ago

@miteshvp Definition of Done: https://docs.google.com/document/d/1S5Z7tPZutOTJEKyYuJCRcKgFkrK2l5mG0HSghTV_ZlY/edit?usp=sharing

this is the 1st version, so please make changes or suggestions to this document