-
Notifications
You must be signed in to change notification settings - Fork 50
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
[DOC] Document setting environment variable #765
Conversation
✅ Deploy Preview for kaleidoscopic-dango-0cf31d canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pavithraes Thanks for documenting this! I've left some comments for your consideration.
Note: this feature is already in conda-store-server, but it's still in review in conda-store-ui. So I'm going to mark this PR as blocked until that one lands, see: conda-incubator/conda-store-ui#354.
@@ -47,11 +47,9 @@ conda-store-server --standalone | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move changes in this file into a separate PR. To me, these look unrelated to the environment variables change, which means they can be merged separately (and likely much faster as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review :)
I did mention in the PR description that it includes some minor, miscellaneous clean-up, so I thought this would be alright.
However, I do agree we should aim for cleaner+specific PRs. I'll revert these updates.
Co-authored-by: Nikita Karetnikov <[email protected]>
@nkaretnikov Thanks for your review, I have addressed all your comments. :) Please take another look and approve it when you get a chance. Of course, even after approval, we'll only merge after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pavithraes Thanks, LGTM! I've left two comments, which would be nice to fix, but those are not blockers. OK to merge this after the companion UI PR lands: conda-incubator/conda-store-ui#354
Co-authored-by: Nikita Karetnikov <[email protected]>
Fixes #759
Description
This pull request:
Pull request checklist
Additional information
Merge after the conda-store-ui feature is merged.