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

📝 Update local-setup-ui.md #6

Merged
merged 14 commits into from
Oct 1, 2024

Conversation

gabalafou
Copy link

@gabalafou gabalafou commented Sep 3, 2024

This is a pull request on top of a pull request: conda-incubator#763.

Related: conda-incubator/conda-store-ui#415.

Kim and Gabriel had a meeting and came up with ideas to make the instructions clearer.

Kim and Gabriel came up with ideas to make this more clear in meeting.
@gabalafou gabalafou changed the title 📝 Update docs contribution guide 📝 Update local-setup-ui.md Sep 3, 2024
@gabalafou
Copy link
Author

gabalafou commented Sep 3, 2024

@kcpevey, I would appreciate if you could check that this PR matches what we discussed in our meeting, as well as spotting if you see any further room for improvement.

@pavithraes, I would appreciate if you could also review the PR so that it aligns with your vision and your base PR, conda-incubator#763.

Copy link

@kcpevey kcpevey left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks @gabalafou

@trallard
Copy link

trallard commented Sep 3, 2024

The first part of this PR says no matter what approach one follows one must install Docker/Docker compose. But then the advance section does not use Docker at all.
Should the Docker install requirements be only needed for container-based development?

Also we should note as requirements installing Docker as it's needed to be able to use Docker compose (right now the only req is Docker compose)

@gabalafou
Copy link
Author

The first part of this PR says no matter what approach one follows one must install Docker/Docker compose. But then the advance section does not use Docker at all.

Heh, this document is starting to feel cursed lol. Both approaches use Docker Compose, but somehow we're failing to communicate all of this in a clear fashion.

The relevant package.json scripts are:

# BASIC
"start:docker": "docker compose --profile local-dev up --build",

# ADVANCED
"start": "yarn run start:services && yarn run start:ui",
"start:services": "docker compose up -d",

The setup labeled "basic" uses start:docker, which in turn calls docker compose and puts everything in Docker, including conda-store-ui. The setup labeled "advanced" uses yarn start which in turn calls start:services which in turn also calls docker compose but runs everything except the conda-store-ui app in a Docker container.

Also we should note as requirements installing Docker as it's needed to be able to use Docker compose (right now the only req is Docker compose)

The installation page for Docker Compose is pretty explicit about needing both Docker Engine and Docker CLI installed in order to use Docker Compose. Also, Docker Compose comes bundled with Docker Desktop for Linux, Mac, and Windows. So I'm not sure how mentioning Docker here as a requirement helps.

Would it be better instead to tell people to install Docker Desktop?

@trallard
Copy link

trallard commented Sep 4, 2024

This is the part that is confusing in the docs as is

For many Conda Store UI development tasks, the basic setup should work. But if you need to work extensively in the UI codebase, then you will probably want to run the conde-store-ui app locally rather than within a Docker container.

So this points to a workflow where one would use a local runtime vs a container.

Indicating that Docker is a requirement is good, since not everyone installs Docker compose from Docker desktop.

@gabalafou gabalafou marked this pull request as draft September 4, 2024 13:46
@gabalafou gabalafou marked this pull request as ready for review September 4, 2024 14:02
docusaurus-docs/community/contribute/local-setup-ui.md Outdated Show resolved Hide resolved
docusaurus-docs/community/contribute/local-setup-ui.md Outdated Show resolved Hide resolved

**Note:** Hot reloading is enabled, so when you make changes to source files, your browser will reload and reflect the changes.

## Run the test suite

We currently use jest in order to run unit tests.
We currently use Jest in order to run unit tests.

```bash
yarn test // find every test with the .test.[tsx|ts] extension
Copy link

Choose a reason for hiding this comment

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

yarn is also still used down here. Not sure about getting rid of all mention of installing yarn?

Copy link

Choose a reason for hiding this comment

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

If one creates the conda environment per the instructions above yarn should be already installed

Copy link
Author

Choose a reason for hiding this comment

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

This section should probably somehow be moved into Advanced development then.

docusaurus-docs/community/contribute/local-setup-ui.md Outdated Show resolved Hide resolved
docusaurus-docs/community/contribute/local-setup-ui.md Outdated Show resolved Hide resolved
docusaurus-docs/community/contribute/local-setup-ui.md Outdated Show resolved Hide resolved
docusaurus-docs/community/contribute/local-setup-ui.md Outdated Show resolved Hide resolved
docusaurus-docs/community/contribute/local-setup-ui.md Outdated Show resolved Hide resolved

**Note:** Hot reloading is enabled, so when you make changes to source files, your browser will reload and reflect the changes.

## Run the test suite

We currently use jest in order to run unit tests.
We currently use Jest in order to run unit tests.

```bash
yarn test // find every test with the .test.[tsx|ts] extension
Copy link

Choose a reason for hiding this comment

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

If one creates the conda environment per the instructions above yarn should be already installed

Copy link
Owner

@pavithraes pavithraes left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!

@@ -9,46 +9,78 @@ To get started with conda-store-ui development, there are a couple of options. T

## Pre-requisites
Copy link
Owner

@pavithraes pavithraes Sep 17, 2024

Choose a reason for hiding this comment

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

If it's Ok with you, I might update this section in my pull request to point to the page that outlines the git cloning & branching workflow: https://github.com/pavithraes/conda-store/blob/contrib-docs/docusaurus-docs/community/contribute/contribute-code.md#setup-for-local-development

Copy link
Author

Choose a reason for hiding this comment

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

That almost makes sense to me, but I'm not sure where/how Docker fits into this suggestion. Should we move the Docker installation instructions out of the core, ui, and juptyerlab-extension docs and into the contribute-code doc?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we can keep Docker here, and only remove the git & GitHub specific instructions in favour of the above doc

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good 👍

Copy link

@trallard trallard left a comment

Choose a reason for hiding this comment

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

This is much better now thanks @gabalafou

@gabalafou
Copy link
Author

I actually realized that this PR does not really depend on conda-incubator/conda-store-ui#415 so I am going to remove that sentence from the PR description.

The instructions in this PR will be wrong if PR 415 is not merged, but they will be wrong in exactly the same way that the current instructions in the docs are wrong. Which is to say, the current instructions for Docker-based dev are missing a yarn install step that really shouldn't be needed but there was a misconfiguration of the Docker Compose file that PR 415 fixes. (For a longer explanation, read the description in conda-incubator/conda-store-ui#415.)

@pavithraes
Copy link
Owner

Got it. This PR is also being merged into another PR, and not the main branch directly. I think we can merge now :)

@pavithraes pavithraes merged commit 6cbcf70 into pavithraes:contrib-docs Oct 1, 2024
2 of 9 checks passed
@pavithraes pavithraes deleted the local-setup-ui branch October 1, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants