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

Update Snyk vulnerabilities in documentation #74

Merged
merged 7 commits into from
Nov 1, 2023

Conversation

owenlittlejohns
Copy link
Member

@owenlittlejohns owenlittlejohns commented Oct 30, 2023

Jira Issue ID

N/A

Description

I was in the Snyk UI and spotted harmony-py has a critical vulnerability in requirements/example.txt. This PR addresses the vulnerability by updating the dependencies for the notebooks in the examples directory. I also bumped the requirements in requirements/docs.txt, as there was a medium severity vulnerability there (tornado via nbconvert).

I checked things still worked by running the notebooks in the examples directory. Most ran, but there were a couple of exceptions:

  • job_stac.ipynb couldn't find pystac.STAC_IO, so I changed that to pystac.stac_io, which seemed to work.
  • The helper.show_results in basics.ipynb failed for a bunch of requests that were getting NetCDF-4 files out of HGA. I fixed this by forcing all the requests to give GeoTIFF outputs.
  • There were some requests made to the Swath Projector that failed message validation. I thought Leo wrote a ticket for this, but can't find it in the DAS backlog. Anyway - that's not an issue with dependencies, but with the service itself.
  • I was lazy and didn't try the in-region parts of the notebooks.

For the Sphinx documentation I had to tweak a method documentation string to make it all run again.

Local Test Steps

For requirements/examples.txt:

  • Pull this branch.
  • Create a new Python environment (via conda or pyenv) and pip install requirements/example.txt.
  • Start a Jupyter notebook server (jupyter lab).
  • Run through the notebooks in the examples directory. Checking basics.ipynb is probably a good one, as it uses a lot of the functionality. Similiarly, the job_stac.ipynb is a good test of pystac and intake-stac.

For requirements/docs.txt:

  • Pull this branch.
  • Create a new Python environment (via conda or pyenv) and pip install requirements/docs.txt.
  • Navigate to the docs directory.
  • Run make html - this should complete successfully.
  • Open the docs/_build/html/index.html file in a browser. It should look like some shiny Sphinx-generated documentation.

PR Acceptance Checklist

  • Acceptance criteria met N/A
  • Tests added/updated (if needed) and passing N/A
  • Documentation updated (if needed)

@owenlittlejohns
Copy link
Member Author

Quick update - I was getting failing builds because I updated the progressbar2 dependency in requirements/docs.txt, but another version was present in requirements/core.txt. I've updated both places, and verified the tests still pass (locally at least)

@@ -7,7 +7,7 @@ jobs:
runs-on: ubuntu-20.04
strategy:
matrix:
python-version: [3.7, 3.8, 3.9, '3.10', '3.11']
Copy link
Member Author

Choose a reason for hiding this comment

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

The update to a recent version of nbconvert requires Python >= 3.8. I also checked and 3.7 was EOL back in June (see: https://devguide.python.org/versions/). It would have been great to not drop 3.7 in the testing, but this seems to be the answer to the rabbit hole of dependencies. (The root of the issue comes from the tests.yml workflow installing the core, test and docs dependencies, and therefore needing all three to be consistent with one another)

@@ -1,6 +1,6 @@
python-dateutil ~= 2.8.2
python-dotenv ~= 0.20.0
progressbar2 ~= 3.55.0
progressbar2 ~= 4.2.0
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to leave these dependencies entirely alone, but progressbar2 needs to be consistent in core.txt and docs.txt to ensure the tests.yml workflow can run. The sphinx dependency in dev.txt was bumped for the same reason.

matplotlib ~= 3.8
netCDF4 ~= 1.6
numpy ~= 1.26
pillow ~= 10.1 # A dependency of ipyplot, pinned to avoid critical vulnerability.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm guessing future releases of ipyplot will bump their own required version of pillow, so this line can be removed at a later date. 🤞

sphinx-rtd-theme ~= 1.2.0
nbconvert ~= 7.10.0
progressbar2 ~= 4.2.0
sphinx ~= 7.1.2
Copy link
Member Author

Choose a reason for hiding this comment

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

sphinx goes up to 7.2.6, but 7.2.0 onwards all require Python 3.9+. 7.1.2 was only released in August, and currently has no vulnerabilities.

Copy link
Member

@ygliuvt ygliuvt left a comment

Choose a reason for hiding this comment

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

Verified the readthedocs looks good.

@owenlittlejohns owenlittlejohns merged commit a006894 into main Nov 1, 2023
10 checks passed
@owenlittlejohns owenlittlejohns deleted the snyk-vulnerabilities-documentation branch November 1, 2023 14:57
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.

3 participants