-
Notifications
You must be signed in to change notification settings - Fork 53
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
Don't build conda store cli #1019
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for conda-store ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thank you @soapy1 this is going in the right direction, I think since you are already going through removing the build for conda-store (cli) we might as well refactor the workflows to remove redundant matrix bits
Another thought we would need to add a note somewhere about stopping releasing the cli as a separate package |
6b0d462
to
baa3929
Compare
There are a few public places that users will be able to see this change:
I think release notes is an appropriate place to be broadcasting this change to communicate with users that this change is coming. Are there any other stream of news that a conda-store user would be looking at to get this type of information? |
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.
Looks great. Did the recipe/meta.yaml
just come from the conda-store-server
conda package?
EDIT: Just saw that they were un-nested from the outputs
section. Sorry!
Nope, I edited it for this PR. We'll need to update the conda-store-server package to match this for next release though. |
a936f10
to
5892e57
Compare
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.
If we are not building the Docker image shall we also remove the Dockerfile?
There does not seem to be a reason to keep straggling files.
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.
I would say it would be better to remove all the conda-store directory at the same time opposed to piecemeal removing pieces.
9b3d8ce
to
f7a66e0
Compare
b3b0325
to
89a2bb5
Compare
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.
There are a lot of places where we use conda-store-server
, which might change as we consolidate the packages. I suggest setting a job-level env variable for this and reusing the variable instead.
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.
I think the amount of effort it will take to introduce a job-level variable will be pretty similar to the amount of effort it will take to explicitly rename things. Since (1) we should only be renaming conda-store-server
-> conda-store
once, and (2) it is easier to read + debug yaml files (and ci) that use explicit names opposed to variables, I would prefer not to introduce this variable.
But open to doing it if I'm missing an important upside that it will provide. I think it would be appropriate to do that work in a follow up PR
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
21a5d54
to
d6a2f63
Compare
part 2 to address #911
Description
This PR removes steps for building and releasing conda-store (the cli). This includes:
cut-release-pr
scriptPull request checklist