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

Privatize internal conda-store API #820

Merged

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented May 6, 2024

Closes #723.

Description

This PR privatizes most of conda-store and conda-store-server in preparation for implementing an upcoming backwards compatibility policy.

This pull request:

  • Moves everything into _internal subpackages, except for a select few outward-facing APIs.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Additional information

How to test

No special testing requirements because new behavior has not been introduced; standard CI tests are sufficient.

Copy link

netlify bot commented May 6, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit 561881e
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/6673289a6081190008a78f19

@peytondmurray peytondmurray force-pushed the privatize-internal-api branch from 79e4366 to 5e45798 Compare May 6, 2024 23:21
@peytondmurray peytondmurray added area: documentation 📖 Improvements or additions to documentation area: api 🌐 status: in progress 🏗 needs: review 👀 impact: high 🟥 This issue affects most of the conda-store users or is a critical issue area: user experience 👩🏻‍💻 Items impacting the end-user experience type: maintenance 🛠 and removed area: documentation 📖 Improvements or additions to documentation area: user experience 👩🏻‍💻 Items impacting the end-user experience labels May 7, 2024
@peytondmurray peytondmurray marked this pull request as ready for review May 7, 2024 07:03
Copy link
Contributor

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

@peytondmurray, I am assuming that every function you left in the pubblic namespace either corresponds to a public rest api route or is used by the conda-store-ui, is that correct. I think this PR is great, I just want to double check that there aren't any little helper functions or anything that we could privatize. They wouldn't need to move to the _internal namespace imo, just have prepend them with a _. I am approving this based on the above assumption, but you might want to take another look through to see if any of the functions that are public could be privatized.

@pavithraes
Copy link
Member

pavithraes commented Jun 11, 2024

@peytondmurray Thanks for the PR.

Moves everything into _internal subpackages, except for a select few outward-facing APIs.

Verified this, and looks good to me. :)

From a design POV, would it make sense to actually move some configuration options (example configs, Nebari's conda-store config) out of _internal? If end-users need to set these config values to deploy conda-store, I'd expect these to be served by our compatibility policy.

@peytondmurray
Copy link
Contributor Author

100%, you're right. I'll move anything that contains subclasses of LoggingConfigurable back out so that they're exposed to the user.

@peytondmurray peytondmurray force-pushed the privatize-internal-api branch 6 times, most recently from 46cea0b to 8fa29ff Compare June 17, 2024 22:30
@peytondmurray peytondmurray force-pushed the privatize-internal-api branch from 8fa29ff to 3f5c7da Compare June 17, 2024 23:13
@peytondmurray peytondmurray merged commit fcac335 into conda-incubator:main Jun 25, 2024
26 of 27 checks passed
@peytondmurray peytondmurray deleted the privatize-internal-api branch June 25, 2024 00:20
peytondmurray added a commit to peytondmurray/conda-store that referenced this pull request Jun 25, 2024
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api 🌐 area: user experience 👩🏻‍💻 Items impacting the end-user experience impact: high 🟥 This issue affects most of the conda-store users or is a critical issue type: maintenance 🛠
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

Privatize conda-store and conda-store-server internals
3 participants