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.

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

Fix for travis CI issue on cryptography dependency #494

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

NajmudheenCT
Copy link
Member

@NajmudheenCT NajmudheenCT commented Feb 25, 2021

What this PR does / why we need it:
A bump in the cryptography indirect dependency lead to cascading breaking changes in our direct dependency pyopenssl. This PR fixes this by directing adding cryptography to our requirements.txt and reverting to a previous cryptography version.

RCA

Newly added cryptography package requires rust compiler .
For more reading about the rust problem in new cryptography version you can read in their github page : pyca/cryptography#5771

Due to this existing travis CI build is failed. reported by some other open source community too
some references
cyberark/cyberark-conjur-cli#183
plotly/dash#1551

In delfin we have cryptography package dependency with pyopenssl . pyopenssl version we use is 19.1 , which supports crypto version 2.8 or above . problematic cryptography version is 3.4.1

Solution

Issue only seems problematic on Python 3.6 . We can attempt use required cryptography version on python 3.6

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
NA
Special notes for your reviewer:

Release note:

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #494 (b876488) into master (8023a1f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #494   +/-   ##
=======================================
  Coverage   67.95%   67.95%           
=======================================
  Files         113      113           
  Lines        8217     8217           
  Branches      926      926           
=======================================
  Hits         5584     5584           
  Misses       2321     2321           
  Partials      312      312           

@NajmudheenCT NajmudheenCT changed the title [Test] test change travis os dist to bionic Fix for travis CI error: change travis os dist to bionic Feb 25, 2021
@sushanthakumar
Copy link
Collaborator

LGTM

@NajmudheenCT NajmudheenCT changed the title Fix for travis CI error: change travis os dist to bionic Fix for travis CI issue on cryptography dependency Mar 2, 2021
@sushanthakumar
Copy link
Collaborator

LGTM

kumarashit
kumarashit previously approved these changes Mar 2, 2021
Copy link
Collaborator

@kumarashit kumarashit left a comment

Choose a reason for hiding this comment

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

LGTM

requirements.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@wisererik wisererik left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@joseph-v joseph-v left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@kumarashit kumarashit left a comment

Choose a reason for hiding this comment

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

LGTM

@kumarashit kumarashit merged commit 4c05408 into sodafoundation:master Mar 4, 2021
joseph-v pushed a commit to joseph-v/delfin that referenced this pull request Mar 8, 2021
* Fix CI Issue on cryptography dependency

* Removing python_version check on crytography package requirement
AmitRoushan pushed a commit to AmitRoushan/delfin that referenced this pull request Mar 9, 2021
* Fix CI Issue on cryptography dependency

* Removing python_version check on crytography package requirement
AmitRoushan pushed a commit to AmitRoushan/delfin that referenced this pull request Mar 9, 2021
* Fix CI Issue on cryptography dependency

* Removing python_version check on crytography package requirement
andrewliu83 referenced this pull request in gh-ca/delfin Mar 11, 2021
* Fix CI Issue on cryptography dependency

* Removing python_version check on crytography package requirement
AmitRoushan pushed a commit to AmitRoushan/delfin that referenced this pull request Mar 12, 2021
* Fix CI Issue on cryptography dependency

* Removing python_version check on crytography package requirement
NajmudheenCT added a commit that referenced this pull request Mar 12, 2021
…e addition (#503)

* db changes for task template addition

* db changes for task instance addition

* Fix for travis CI issue on cryptography dependency (#494)

* Fix CI Issue on cryptography dependency

* Removing python_version check on crytography package requirement

* Add MIXED storage pool type (#482)

* Add NAS resources for filesystem, qtree & share (#491)

* Add NAS resource filesystem

* Separate drivers to NAS, SAN and Unified

* Fix formatting

* Add Qtree resource support to delfin

* Add Share resource support to delfin

* Add quota limit params to qtree

* Update unit tests

* Update model changes

* Implement review comments

* update db model with task and failed task schema

* Fix some comments and name changes

* Unit test changes

Co-authored-by: Najmudheen <45681499+NajmudheenCT@users.noreply.github.com>
Co-authored-by: Joseph Vazhappilly <josephvp@gmail.com>
Co-authored-by: Amit Roushan <amit.roushan@huawei.com>
NajmudheenCT added a commit to NajmudheenCT/dolphin-1 that referenced this pull request Mar 15, 2021
* Fix CI Issue on cryptography dependency

* Removing python_version check on crytography package requirement
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.

5 participants