Skip to content

Latest Version Changes - #235

Open
yzainee-zz wants to merge 3 commits into
fabric8-analytics:masterfrom
yzainee-zz:notify
Open

Latest Version Changes#235
yzainee-zz wants to merge 3 commits into
fabric8-analytics:masterfrom
yzainee-zz:notify

Conversation

@yzainee-zz

Copy link
Copy Markdown
Member

No description provided.

@yzainee-zz
yzainee-zz requested a review from msrb February 1, 2019 11:36
@codecov-io

codecov-io commented Feb 7, 2019

Copy link
Copy Markdown

Codecov Report

Merging #235 into master will decrease coverage by 0.49%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #235     +/-   ##
=========================================
- Coverage   92.64%   92.15%   -0.5%     
=========================================
  Files          10       10             
  Lines         884      892      +8     
=========================================
+ Hits          819      822      +3     
- Misses         65       70      +5
Impacted Files Coverage Δ
src/data_importer.py 90.33% <100%> (+0.14%) ⬆️
src/graph_manager.py 100% <100%> (ø) ⬆️
src/cve.py 84.25% <100%> (ø) ⬆️
src/data_source/s3_data_source.py 94.44% <100%> (ø) ⬆️
src/rest_api.py 89.06% <100%> (-1.05%) ⬇️
src/utils.py 100% <100%> (ø) ⬆️
src/graph_populator.py 96.72% <85.71%> (-1.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3bfb0d...099abbf. Read the comment docs.

Comment thread src/graph_populator.py Outdated
"property('{ecosystem}_pkg_count',1)).iterate();" \
"graph.addVertex('ecosystem', '{ecosystem}', " \
"'name', '{pkg_name}', 'vertex_label', 'Package');}};" \
"pkg.property('latest_version', {latest_version});" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yzainee - you are missing ' single quote around latest_version here. That's why tests are failing

@msrb msrb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please squash some of the commits in this PR ("Initial commit" for example).

There are some scary parts that still deal with libio latest version etc., for example:

if libio_latest_version != cur_libio_latest_ver and last_updated_flag != 'true':
prp_package += "pkg.property('latest_version_last_updated', '{}');"\
.format(cur_date)

Are we sure it won't break any use cases?

I think it would be better to do some refactoring and try to figure out from where to call get_latest_versions_for_ep, so we don't need to call it 3 times.

Comment thread Dockerfile.data-model
# Note: cron daemon ( crond ) will be invoked from within entry point
# --------------------------------------------------------------------------------------------------------------
RUN pip3 install git+https://git@github.com/fabric8-analytics/fabric8-analytics-version-comparator.git
RUN pip3 install git+https://github.com/fabric8-analytics/fabric8-analytics-utils.git

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't be better to install specific versions of these dependencies?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes i will make those changes in the final commit. Still its WIP as the tests are not passing.

# Note: cron daemon ( crond ) will be invoked from within entry point
# --------------------------------------------------------------------------------------------------------------
RUN pip3 install git+https://git@github.com/fabric8-analytics/fabric8-analytics-version-comparator.git
RUN pip3 install git+https://github.com/fabric8-analytics/fabric8-analytics-utils.git@latest

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@latest?

Comment thread requirements.txt Outdated
urllib3==1.24.1 # via botocore, minio, requests
uuid==1.30
werkzeug==0.14.1 # via flask, pytest-flask
git+https://git@github.com/fabric8-analytics/fabric8-analytics-version-comparator.git

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we install the dependencies here, do we need to install them in the Dockerfiles separately?

Comment thread setup.py Outdated
@@ -0,0 +1,59 @@
#!/usr/bin/env python3

# Copyright © 2018 Red Hat Inc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Happy New Year! :)

Comment thread src/graph_populator.py
str_gremlin += str_gremlin_version
if not prp_package:
# TODO: refactor into the separate module
latest_version = get_latest_versions_for_ep(ecosystem, pkg_name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, why do we call get_latest_versions_for_ep on 3 different places in this PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is my biggest issue with this PR :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The reason is that different APIs are triggering different functional flows, where gremlin queries are getting generated for the package node. So basically thats 2 of them. The 3rd one, construct graph node function, i have added it so that even in cases where we are creating dummy nodes during CVEs etc, the package node should get updated to the latest version. This is an extra check added to make sure that our package node is always updated.

@centos-ci

Copy link
Copy Markdown
Collaborator

@yzainee Your image is available in the registry: docker pull quay.io/openshiftio/rhel-bayesian-data-model-importer:SNAPSHOT-PR-235

1 similar comment
@centos-ci

Copy link
Copy Markdown
Collaborator

@yzainee Your image is available in the registry: docker pull quay.io/openshiftio/rhel-bayesian-data-model-importer:SNAPSHOT-PR-235

tisnik and others added 2 commits February 15, 2019 13:07
Lock to Radon 3 0 1

initial commit

initial commit

Changes for latest version

cico script changed for python3

added a dummy node for latest version

pylint

tests rectification

fixing tests

review comments
@yzainee-zz yzainee-zz changed the title Notify Latest Version Changes Feb 15, 2019
@tisnik

tisnik commented Feb 18, 2019

Copy link
Copy Markdown
Member

[test]

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.

6 participants