-
Notifications
You must be signed in to change notification settings - Fork 49
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] Update contributing guidelines #763
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. |
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
TODO: Incorporate updates from #792 |
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
Kim and Gabriel came up with ideas to make this more clear in meeting.
Co-authored-by: Kim Pevey <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
One follow-up task for this PR right after it is merged:
|
📝 Update local-setup-ui.md
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
Signed-off-by: Pavithra Eswaramoorthy <[email protected]>
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 @pavithraes! I made some suggestions.
Also @peytondmurray the part about the tests seems too complex, please see the relevant comment as it would be best to simplify this workflow.
Most of the conda-store docker images are built/tested for amd64(x86-64). Notice the `architecture: amd64` within the `docker-compose.yaml` files. | ||
|
||
There will be a performance impact when building and running on | ||
arm architectures. Otherwise, this workflow has been shown to run and build on OSX. |
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.
arm architectures. Otherwise, this workflow has been shown to run and build on OSX. | |
ARM architectures. Otherwise, this workflow has been shown to run and build on OSX. |
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.
Otherwise, this workflow has been shown to run and build on OSX.
Assume here we refer to series M?
```bash | ||
hatch env run -e dev playwright-test | ||
hatch env run -e dev integration-test | ||
``` |
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.
We have many ways to run the tests, hatch
, pytest
and calling bash scripts. At least for the Python ones we should stick to only one (either through hatch
or pytest
directly).
I would suggest hatch
as we can also configure it so that we call the bash scripts with hatch
https://hatch.pypa.io/latest/config/environment/overview/#scripts and set the env variables there too https://hatch.pypa.io/latest/config/environment/overview/#environment-variables
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.
☝️ Note that we will be deprecating these soon when the old admin UI is removed. It doesn't matter to me much what approach we take, this is something we can easily change later on, though I will pretty much always vote for a simple pytest
invocation because it's what most folks will be familiar with.
|
||
:::note | ||
|
||
Hot reloading is enabled, so when you make changes to source files (i.e., files under the conda-store-ui/src/ directory), your browser will reload and reflect the changes. |
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.
Hot reloading is enabled, so when you make changes to source files (i.e., files under the conda-store-ui/src/ directory), your browser will reload and reflect the changes. | |
Hot reloading is enabled, so when you change source files (files under the conda-store-ui/src/ directory), your browser will reload and reflect the changes. |
|
||
:::note | ||
|
||
This setup still uses Docker to run the rest of the Conda Store stack. That means that the Conda Store database, server, worker, and storage services will all run in Docker containers. However, the frontend web app (conda-store-ui) will run locally (not in a Docker container) with this setup. |
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.
This setup still uses Docker to run the rest of the Conda Store stack. That means that the Conda Store database, server, worker, and storage services will all run in Docker containers. However, the frontend web app (conda-store-ui) will run locally (not in a Docker container) with this setup. | |
This setup still uses Docker to run the rest of the conda-store stack. That means the conda-store database, server, worker, and storage services will all run in Docker containers. However, the frontend web app (conda-store-ui) will run locally (not in a Docker container) with this setup. |
|
||
1. Activate the development environment: `conda activate cs-ui-dev-env` | ||
2. Set environment variables: copy `.env.example` to `.env`. All default settings should work, but if you want to test against a different version of conda-store-server, you can specify it in the `.env` file by setting the `CONDA_STORE_SERVER_VERSION` variable to the desired version. | ||
3. Run: `corepack enable`. Corepack comes with Node.js which was installed when you set up your Conda dev environment. Running `corepack enable` ensures that the version of Yarn that you use in the next step matches the version required by the repo (see package.json > packageManager). Yarn also comes with Node.js but is not automatically added to your environment's path until you run `corepack enable`. |
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.
3. Run: `corepack enable`. Corepack comes with Node.js which was installed when you set up your Conda dev environment. Running `corepack enable` ensures that the version of Yarn that you use in the next step matches the version required by the repo (see package.json > packageManager). Yarn also comes with Node.js but is not automatically added to your environment's path until you run `corepack enable`. | |
3. Run: `corepack enable`. Corepack comes with Node.js, which is installed when you set up the conda environment. Running `corepack enable` ensures that the version of Yarn that you use in the next step matches the version required by the repo (see `package.json > packageManager`). Yarn also comes with Node.js but is not automatically added to your environment's path until you run `corepack enable`. |
2. Set environment variables: copy `.env.example` to `.env`. All default settings should work, but if you want to test against a different version of conda-store-server, you can specify it in the `.env` file by setting the `CONDA_STORE_SERVER_VERSION` variable to the desired version. | ||
3. Run: `corepack enable`. Corepack comes with Node.js which was installed when you set up your Conda dev environment. Running `corepack enable` ensures that the version of Yarn that you use in the next step matches the version required by the repo (see package.json > packageManager). Yarn also comes with Node.js but is not automatically added to your environment's path until you run `corepack enable`. | ||
4. Install/update JavaScript dependencies: `yarn install` | ||
5. Run `yarn run start` and wait for the application to finish starting up. This command will run a local dev server for the UI app and run the other Conda Store services in Docker. |
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.
5. Run `yarn run start` and wait for the application to finish starting up. This command will run a local dev server for the UI app and run the other Conda Store services in Docker. | |
5. Run `yarn run start` and wait for the application to finish starting up. This command will run a local dev server for the UI app and run the other conda-store services in Docker. |
|
||
### Run the test suite | ||
|
||
If you chose the advanced option for local development, you will be able to run the tests locally. We currently use Jest in order to run unit tests. |
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 you chose the advanced option for local development, you will be able to run the tests locally. We currently use Jest in order to run unit tests. | |
If you choose the advanced option for local development, you can run the tests locally. We currently use Jest to run unit tests. |
@pavithraes think this only needs some minor changes and we can merge |
I would also like to see this PR merged soon. I have some docs updates I would like to make but I have been holding back for this PR to be merged. |
Addresses #748
Description
This pull request:
Pull request checklist
Additional information
Squash merge this PR.
How to test