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.

Sign up for GitHub

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

Jump to bottom

Move/deprecate conda_env modules into conda #13168

Merged
merged 89 commits into from
Dec 22, 2023
Merged

Move/deprecate conda_env modules into conda #13168

merged 89 commits into from
Dec 22, 2023

Conversation

beeankha
Copy link
Member

@beeankha beeankha commented Oct 3, 2023 •
edited
Loading

This PR addresses the following epic:

To do:

  • Add test to /conda/tests/conda_env/test_cli.py to test conda_env function imports work as expected (before moving anything around)
  • Move conda_env modules into conda
  • Add tests to check that redirect imports (from moved modules to deprecated modules) are still intact and working

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@beeankha beeankha added source::anaconda created by members of Anaconda, Inc. type::tech-debt identifies or resolves some technical debt labels Oct 3, 2023
@beeankha beeankha self-assigned this Oct 3, 2023
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 3, 2023
@beeankha beeankha force-pushed the conda-env-collapse branch 3 times, most recently from fad6b4f to 10d2599 Compare October 6, 2023 20:49
@beeankha beeankha marked this pull request as ready for review October 11, 2023 15:44
@beeankha beeankha requested a review from a team as a code owner October 11, 2023 15:44
kenodegard
kenodegard requested changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Some broad stroke observations:

  1. all new deprecations are for 24.3 and 24.9 (not 23.9 and 24.3)
  2. all functions, classes, and modules marked as deprecated from before should not move to conda.env
  3. need to combine conda.env.cli with conda.cli (common is probably the easiest to do, can wait with main for now)

@beeankha beeankha marked this pull request as draft October 11, 2023 16:16
@beeankha beeankha force-pushed the conda-env-collapse branch 4 times, most recently from 9afa89b to 1227e5b Compare October 16, 2023 22:44
@beeankha beeankha marked this pull request as ready for review October 25, 2023 19:38
@travishathaway
Copy link
Contributor

@beeankha,

I think this looks good so far. Before I give my approval, I want to clone this locally and poke around at it though. Please wait to merge until I have done this 🙏.

@jezdez jezdez self-requested a review October 28, 2023 09:28
@travishathaway
Copy link
Contributor

Got a chance to poke around at it more...

One thought I had while I was looking through it is why we aren't we taking the chance to combine the conda envArgumentParser object with the conda ArgumentParser object? If we did so, we could eliminate the need for this file:

I'm okay if that's too much to pack into a single pull request, but I would like to know the plans for eventually getting rid our usage of that file. It would be really nice for us to to have a single ArgumentParser object for the project.

kenodegard
kenodegard requested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Looking good!

Now to eliminate unnecessary code paths (e.g. calling conda env still invokes the conda-env entrypoint which in turn imports from conda_env etc.).

Thoughts on moving tests/conda_env to tests/env and moving all of the new import tests into a new tests/test_conda_env.py?

conda_env/__main__.py Outdated Show resolved Hide resolved
news/13168-collapse-conda-env Outdated Show resolved Hide resolved
tests/cli/test_subcommands.py Outdated Show resolved Hide resolved
conda_env/cli/main.py Outdated Show resolved Hide resolved
@beeankha
Copy link
Member Author

beeankha commented Nov 2, 2023

Thoughts on moving tests/conda_env to tests/env and moving all of the new import tests into a new tests/test_conda_env.py?

This is a good idea, implemented in latest commit 👍

@jezdez jezdez mentioned this pull request Nov 8, 2023
Merged
3 tasks
jezdez
jezdez requested changes Nov 8, 2023
View reviewed changes
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

Just a small nit that would be dedicated to help with #13291 (comment)

tests/env/test_cli.py Outdated Show resolved Hide resolved
kenodegard
kenodegard requested changes Nov 8, 2023
View reviewed changes
conda/cli/helpers.py Outdated Show resolved Hide resolved
conda/cli/common.py Outdated Show resolved Hide resolved
conda/cli/main_env.py Outdated Show resolved Hide resolved
conda/cli/main_env_create.py Outdated Show resolved Hide resolved
conda/cli/main_env_export.py Outdated Show resolved Hide resolved
conda/cli/main_env_list.py Outdated Show resolved Hide resolved
conda/cli/main_env_update.py Outdated Show resolved Hide resolved
conda/testing/__init__.py Outdated Show resolved Hide resolved
@beeankha beeankha force-pushed the conda-env-collapse branch 3 times, most recently from 9f5a083 to 961728e Compare November 10, 2023 19:28
@beeankha beeankha mentioned this pull request Nov 13, 2023
Merged
jezdez
jezdez previously approved these changes Dec 18, 2023
View reviewed changes
kenodegard
kenodegard previously approved these changes Dec 18, 2023
View reviewed changes
travishathaway
travishathaway previously approved these changes Dec 19, 2023
View reviewed changes
jezdez
jezdez previously approved these changes Dec 19, 2023
View reviewed changes
@jezdez jezdez dismissed stale reviews from travishathaway, kenodegard, and themself via 5ffef00 December 19, 2023 07:50
@jezdez jezdez enabled auto-merge (squash) December 19, 2023 07:52
@jezdez
Copy link
Member

jezdez commented Dec 19, 2023

Minor merge conflicts due to #13433 that I just fixed. The PR has obviously been approved a few times now.

@jezdez jezdez merged commit 33a75a9 into conda:main Dec 22, 2023
73 checks passed
@jezdez jezdez mentioned this pull request Dec 22, 2023
Closed
@kenodegard kenodegard mentioned this pull request Dec 22, 2023
Merged
3 tasks
@travishathaway travishathaway mentioned this pull request Dec 28, 2023
Closed
3 tasks
@kenodegard kenodegard mentioned this pull request Jan 30, 2024
Closed
56 tasks
This was referenced Feb 19, 2024
Merged
Merged
This was referenced Mar 26, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenodegard kenodegard kenodegard approved these changes

@jezdez jezdez jezdez left review comments

@travishathaway travishathaway travishathaway left review comments

Assignees

@beeankha beeankha

Labels
cla-signed [bot] added once the contributor has signed the CLA source::anaconda created by members of Anaconda, Inc. type::tech-debt identifies or resolves some technical debt
Projects
🔎 Review
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@beeankha @travishathaway @kenodegard @jezdez @conda-bot