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

DAS-2180: Remove Conda dependency on docker images #20

Merged
merged 5 commits into from
Jun 20, 2024

Conversation

flamingbear
Copy link
Member

@flamingbear flamingbear commented Jun 18, 2024


Description

Remove conda dependency from HyBIG

Updates the docker images to use python:3.11 instead of continuumio/miniconda3 and removes conda commands and environment

Jira Issue ID

DAS-2180

Local Test Steps

Pull branch, build and deploy to Harmony-In-A-Box

./bin/build-image && ./bin/build-test && ./bin/run-test

Make sure unit tests pass.

Run regression tests against the local image.

PR Acceptance Checklist

  • [N/A] Jira ticket acceptance criteria met.
  • CHANGELOG.md updated to include high level summary of PR changes.
  • docker/service_version.txt updated if publishing a release.
  • [N/A] Tests added/updated and passing.
  • [N/A] Documentation updated (if needed).

@flamingbear
Copy link
Member Author

So b854d46 may be the worst commit I've ever done. All it does is move the uninstallable snyk package into the file that is ignored in snyk. And confusingly still calls it all conda when it's all pip.

@flamingbear flamingbear force-pushed the mhs/DAS-2180/remove-conda-requirement branch from b854d46 to b5599f9 Compare June 18, 2024 20:47
Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

First up - I love the migration to a single package manager, this is great.

As discussed in Slack I am concerned about this change preventing us from scanning any dependencies in Snyk, because it can't install gdal when scanning. I think we landed on your alternative, which was to put the gdal dependency in a separate file, which Snyk doesn't scan. Kind of ugly, but I think that gets us to a better place overall. We were trying to come up with a good file name. How about: pip_requirements_non_snyk.txt (not particularly great, but 🤷). Probably would be good to add prose to the README and the top of that file just explaining why it exists, and how/when it might be removable.

Also noting that pip install can support multiple files via -r in the same command for when you add that to this PR.

@flamingbear
Copy link
Member Author

@owenlittlejohns I think this is ready for re-review.

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

Nice! I do like some of the things this work has enabled the removal of - like those environment variables that were set to activate the conda environment (while continuing to allow log streaming).

@flamingbear flamingbear merged commit 42b3240 into main Jun 20, 2024
4 checks passed
@flamingbear flamingbear deleted the mhs/DAS-2180/remove-conda-requirement branch June 20, 2024 20:41
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.

2 participants