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

WX-1192 Updated semver versions for flagged dependencies #790

Merged
merged 5 commits into from
Aug 2, 2023
Merged

Conversation

JVThomas
Copy link
Collaborator

@JVThomas JVThomas commented Jul 27, 2023

Addresses WX-1192

PR updates the semver dependency in sass to 7.5.2 in order to patch out a CVE present in older versions.

There's also an additional backend update (unrelated to Sass) that was required due to a breaking sub-dependency update on PyYAML. A constraints file was added to constrain Cython's (the sub-dependency) version.

Copy link

@THWiseman THWiseman left a comment

Choose a reason for hiding this comment

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

I think it would be good if we left some sort of comment, somewhere, explaining how we're messing with python and why (and maybe a link to that github issue). I can see something like this coming up again...

@@ -0,0 +1 @@
cython<3.0.0

Choose a reason for hiding this comment

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

Will this constraint apply to every python package? I'm worried that it will end up causing more problems down the road as other packages begin to rely on new cython features. Is it possible to constrain just the problematic package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it will, however we pin dependencies to their exact values. If a fixed dependency updates it's Cython sub-dependency from v2 to v3 and still uses the deprecated features of Cython v2 then it'll break as well.

Constraining the version won't change the functionality of the library, and new features on a library would be tied to a version update which JM won't see due to pinned versions.

Choose a reason for hiding this comment

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

That makes sense.

@@ -2,6 +2,7 @@
envlist = py310

[testenv]
setenv=PIP_CONSTRAINT={toxinidir}/constraints.txt

Choose a reason for hiding this comment

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

I believe it's good practice to include the constraints.txt inside a particular requirements.txt. I think it could make it a little easier down the road to figure out what is being constrained and why. This article seems helpful. Not a huge deal if there's some reason we can't do that.

Copy link
Collaborator Author

@JVThomas JVThomas Jul 28, 2023

Choose a reason for hiding this comment

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

I went with the above per tox documentation here, which outlines setting constraints as seen above.

Didn't know you could do the above (most Github posts have people adding the -c <file name> flag to pip install), but seeing how it conflicts with the documented usage in tox I've opted for this instead.

That said I do need to update any pip install references to include the constraints flag. I'll add a new commit that does it.

Choose a reason for hiding this comment

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

I'm in favor of doing what tox documentation does.

Good catch on the pip/docker stuff!

Copy link

@THWiseman THWiseman left a comment

Choose a reason for hiding this comment

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

LGTM!

jgainerdewar
jgainerdewar previously approved these changes Jul 28, 2023
README.md Outdated
@@ -105,7 +105,7 @@ Monitors jobs launched by the [Cromwell workflow engine](https://github.com/broa
#### Notes
1. Websocket reload on code change does not work in docker-compose (see
https://github.com/angular/angular-cli/issues/6349).
2. Changes to `package.json` or `requirements.txt` or [regenerating the API](#updating-the-api-using-swagger-codegen) require a rebuild with:
2. Changes to `package.json`, `requirements.txt` or `constraints.txt` [regenerating the API](#updating-the-api-using-swagger-codegen) require a rebuild with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to go with your preference here but can't miss this chance to stand up for the oxford comma.

@JVThomas
Copy link
Collaborator Author

JVThomas commented Aug 1, 2023

Sorry, I'm going to ask for a re-review on my PR. While tox was able to handle the backend installation correctly, building the Docker image locally did not. Further investigation led me to this comment:

yaml/pyyaml#736 (comment)

Turns out that using the -c flag on pip install does not work properly if the package is installed through a wheel. In order to ensure that the constraints file is respected, the file itself must be assigned to thePIP_CONSTRAINT env variable alongside the individual install of PyYAML via pip install (PyYAML is also commented out of the requirements.txt file.

Once the above completes, the Dockerfile will install the rest of the packages via requirements.txt

@JVThomas JVThomas dismissed jgainerdewar’s stale review August 1, 2023 17:24

Updated PR, requesting re-review

@JVThomas JVThomas merged commit 5388162 into master Aug 2, 2023
2 checks passed
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