Skip to content
This repository has been archived by the owner on May 2, 2024. It is now read-only.

Simplify workflow for mounting repositories from host #43

Closed
kdmccormick opened this issue Feb 1, 2022 · 9 comments · Fixed by overhangio/tutor#643
Closed

Simplify workflow for mounting repositories from host #43

kdmccormick opened this issue Feb 1, 2022 · 9 comments · Fixed by overhangio/tutor#643
Assignees
Labels
enhancement Relates to new features or improvements to existing features

Comments

@kdmccormick
Copy link
Collaborator

kdmccormick commented Feb 1, 2022

Context

Desired workflow:

  1. Make a branch off of edx-platform master and/or check out an existing master-based branch.
  2. Make changes to the branch.
  3. Run linting & tests using Tutor.
  4. Manually try out the changes using Tutor.
  5. Go to (2)

The docs recommend using tutor dev bindmount to extract a provisioned clone of edx-platform from the Tutor LMS container. I hit several issues:

  • The 'bindmount' terminology confused me at first. The command does not mount a volume, but instead extracts a directory for the purpose of mounting as a volume.
  • The extracted edx-platform clone was not at master, but instead was at master plus a bunch of patches and some dirty git state (copied static assets, I think).
  • Branches were not included in the clone, so I had tell git explicitly to fetch all branches.
  • The origin protocol was https instead of ssh, so I could not push code until updating the origin.
  • Working out of $(tutor config printroot)/volumes/edx-platform was a bit of pain. I ended linking the repository to my home directory, but that's a lot of steps to ask a new developer to do.
  • Adding --volume=/openedx/edx-platform to every command was cumbersome. Being able to add the mounts to docker-compose.override.yml was a lot better, but still one more step.

I ended up avoiding tutor dev bindmount after that and instead just did this:

cd ~/openedx && git clone git@github.com:openedx/edx-platform

# Configure two custom bindmounts for LMS and CMS:
#   ~/openedx/edx-platform -> /openedx/edx-platform
#   ~/openedx/edx-platform-venv -> /openedx/venv
vi $(tutor config printroot)/env/dev/docker-compose.override.yml   

tutor dev run lms make requirements
tutor dev run lms npm install
tutor dev init
tutor dev run lms openedx-assets --env=dev

after which it was smooth sailing.

@regisb, was I using tutor dev bindmount wrong, and is there anything wrong with the workflow I ended up on? Either way, could we make it easier to get started with working on branches off of master?

Acceptance

  1. Add a new option to start (proposed names: --automount, --mount). The option can be supplied multiple times in one command. Each use of the option should take an argument in one of the following forms:
    1. HOST_PATH (automatic mount)
    2. SERVICE1,SERVICE2:HOST_PATH:CONTAINER_PATH (manual mount)
  2. For the automatic mount case, define a new filter which takes a dir_name argument and produces a list of (service, container_path) tuples. Use this new filter to determine where directories should be auto-mounted, based on the basename of the directory.
  3. Add the --mount option with the same interface to Tutor's run command.
  4. Add the --mount option with the same interface to Tutor's init command.
  5. To avoid confusion with the new mounting system, consider renaming bindmount to something more fitting. Ideas: extract, extractmount, copyfrom.
  6. Update documentation on Open edX development, bindmounting, etc.

WIP implementation

was started here: kdmccormick/tutor#8

@kdmccormick
Copy link
Collaborator Author

@regisb This one is also ready for a look.

@regisb
Copy link

regisb commented Feb 15, 2022

was I using tutor dev bindmount wrong, and is there anything wrong with the workflow I ended up on? Either way, could we make it easier to get started with working on branches off of master?

No, you were doing things just right. I agree that the "bindmount" workflow is too complex. A proper solution would be to declare all bind-mounts in the docker-compose*.yml files. This would contribute to getting rid of the docker-compose run commands (see #26).

Thus, the question becomes: how do we make it easier to add volumes to the docker-compose*.yml files? The current solution is to manually edit the docker-compose.override.yml files (as you are doing): https://docs.tutor.overhang.io/dev.html#override-docker-compose-volumes The problem with the docker-compose.override.yml file is that it's a bit difficult to rollback changes. It's not very convenient when you want to bind-mount a repo for a one-off debug command. Also, we do not make it clear to the user at runtime which volumes are mounted. This goes agains the policy of tutor to make things explicit as much as possible.
I'm not sure how to improve that workflow... Yet it is essential for a good developer onboarding experience. (I'm thinking out loud here...)

Could we consider the following syntax?

tutor dev start -d --automount /path/to/edx-platform --automount /path/to/course-discovery

With such a command, every repository would auto-magically be bind-mounted to the right folder in the right container(s). With the plugin v1 API, I believe that we could have a rather lean implementation of that feature and make every plugin responsible for its own set of bind-mounts.
In case we want to be more explicit, we could use a different syntax:

# Bind-mount the /path/to/edx-platform folder to /openedx/edx-platform in the lms, cms, lms-worker, cms-worker containers.
tutor dev start -d --automount lms,cms,lms-worker,cms-worker:/openedx/edx-platform:/path/to/edx-platform ...

The automount option would generate a docker-compose.volumes.yml file that would be updated at every run.

Preparation of the edx-platform repo (make requirements && npm install, etc.), could be made easier with the simple addition of a script or make command.

@kdmccormick
Copy link
Collaborator Author

kdmccormick commented Feb 15, 2022

The problem with the docker-compose.override.yml file is that it's a bit difficult to rollback changes. It's not very convenient when you want to bind-mount a repo for a one-off debug command. Also, we do not make it clear to the user at runtime which volumes are mounted. This goes agains the policy of tutor to make things explicit as much as possible.
I'm not sure how to improve that workflow... Yet it is essential for a good developer onboarding experience. (I'm thinking out loud here...)

+1 to everything here ^

Preparation of the edx-platform repo (make requirements && npm install, etc.), could be made easier with the simple addition of a script or make command.

👍🏻

Could we consider the following syntax?
tutor dev start -d --automount /path/to/edx-platform --automount /path/to/course-discovery

Interesting! The syntax itself looks user friendly. I am curious about how the auto-magic part would work, though. Would tutor/plugins would need to look at the contents of each automounted folder to deduce where it should be bind-mounted in which containers?

In a different direction, how would you feel about using configuration settings to set up bind-mounts? Something like this: kdmccormick/tutor#2. Then, the workflow would end up like this:

tutor config save --set OPENEDX_MOUNT_REPOSITORY=/path/to/edx-platform
tutor config save --set DISCOVERY_MOUNT_REPOSITORY=/path/to/course-discovery
tutor start -d lms cms discovery
tutor exec lms make requirements static
tutor exec discovery make requirements static

Plugins would be responsible for exposing settings allowing bind-mounting of repositories or other resources.

Either way, I think we'd want to leave the docker-compose.override.yml route in place in order to give advanced users an escape hatch into bind-mounting whatever they need to.

Let me know your thoughts.

@kdmccormick kdmccormick changed the title As a nightly user, I want to make local code changes in fewer steps Simplify workflow for mounting repositories from host Feb 15, 2022
@regisb
Copy link

regisb commented Feb 21, 2022

I am curious about how the auto-magic part would work, though. Would tutor/plugins would need to look at the contents of each automounted folder to deduce where it should be bind-mounted in which containers?

Yes, exactly. There would be special logic in place for every plugin that would capture some of the --automount arguments. This would be made possible in an elegant way in the plugins v1 API.

In a different direction, how would you feel about using configuration settings to set up bind-mounts?

I have thought about this but I'm feeling reluctant. The reason is that tutor configuration and environments are portable. You can move them around from one computer to the next and they will keep working. If we add machine-specific information there, then we will lose that portability.

Either way, I think we'd want to leave the docker-compose.override.yml route in place in order to give advanced users an escape hatch into bind-mounting whatever they need to.

Yes, absolutely.

@kdmccormick
Copy link
Collaborator Author

kdmccormick commented Feb 22, 2022

I have thought about this but I'm feeling reluctant. The reason is that tutor configuration and environments are portable.

That makes sense. I wouldn't want to break that simplifying assumption.

The reason I suggested using configuration was to find a syntax more concise than:

tutor dev start -d --automount /path/to/edx-platform --automount /path/to/course-discovery

But, if we allow relative paths and let -a be an abbreviation for --automount, the resulting command syntax seems pretty agreeable to me:

cd /path/to
tutor dev start -d -a edx-platform -a course-discovery

And for the average dev, who is working on one IDA at a time, the common startup command will just be:

cd /path/to/repo
tutor dev start -d -a .

In case we want to be more explicit, we could use a different syntax:

tutor dev start -d --automount lms,cms,lms-worker,cms-worker:/openedx/edx-platform:/path/to/edx-platform ...

Hm, instead of being another form of --automount, this strikes me as a useful but completely different command option. As far as I can tell, it would not be subject to the special logic in each plugin--the directory would just get mounted to whichever containers and locations were specified. It seems more like --mount, or maybe a generalization of the existing --volume option from the run command interface. Update: I changed my mind on this. I think it'd be best to have one option for both formats.

@kdmccormick kdmccormick added enhancement Relates to new features or improvements to existing features and removed for:2u-pilot labels Feb 22, 2022
@regisb regisb mentioned this issue Apr 1, 2022
6 tasks
@kdmccormick
Copy link
Collaborator Author

WIP here: kdmccormick/tutor#8

@kdmccormick kdmccormick moved this from Ungroomed (Régis) to In Progress in Tutor DevEnv Adoption (OLD BOARD) Apr 14, 2022
@kdmccormick kdmccormick self-assigned this Apr 14, 2022
@kdmccormick kdmccormick moved this to In Progress in Axim Engineering Tasks Apr 14, 2022
@kdmccormick
Copy link
Collaborator Author

kdmccormick commented Apr 14, 2022

@regisb I started hacking on this because I think it'd be great to be able to showcase it in the workshop. Here's my first stab at the helptext, which explains the precise syntax I'm implementing. It's almost exactly the same syntax you suggested, but I'm proposing calling it just "mount" instead of "automount", since the long, manual SERVICES:PATH:TARGET form the command doesn't have the "automatic" aspect we discussed. Let me know if you disagree.

tutor dev start --help
Usage: tutor dev start [OPTIONS] service

  Run all or a selection of services. Docker images will be rebuilt where
  necessary.

Options:
  --skip-build                    Skip image building
  -d, --detach                    Start in daemon mode
  -m, --mount [SERVICES:]PATH[:TARGET]
                                  Bind-mount the host directory PATH.
                                  Optionally specify comma-separated SERVICES
                                  and a TARGET path; if omitted, they are
                                  deduced from the directory's name.
  --help                          Show this message and exit.

@kdmccormick kdmccormick assigned regisb and unassigned kdmccormick Apr 14, 2022
@kdmccormick kdmccormick moved this from In Progress to Groomed in Tutor DevEnv Adoption (OLD BOARD) Apr 14, 2022
@kdmccormick
Copy link
Collaborator Author

@regisb I added an acceptance criteria and assigned this one to you. If you find you're too busy to finish this next week, let me know, and I'd be happy to try to finish it in time for the conference.

@regisb regisb moved this from Groomed to In Progress in Tutor DevEnv Adoption (OLD BOARD) Apr 15, 2022
@kdmccormick kdmccormick moved this from In Progress to Done in Axim Engineering Tasks Apr 18, 2022
regisb added a commit to overhangio/tutor that referenced this issue Apr 19, 2022
The `--mount` and `--automount` options are available both with `tutor local`
and `tutor dev` commands. They allow users to easily bind-mount containers from
the host to containers. Yes, I know, we already provide that possibility with
the `bindmount` command and the `--volume=/path/` option. But these suffer from
the following drawbacks:

- They are difficult to understand.
- The "bindmount" command name does not make much sense.
- It's not convenient to mount an arbitrary folder from the host to multiple
  containers, such as the many lms/cms containers (web apps, celery workers and
  job runners).

To address this situation, we now recommend to make use of --mount and
--automount options.

1. `--mount=service1[,service2,...]:/host/path:/container/path`: manually mount
   `/host/path` to `/container/path` in container "service1" (and "service2").
2. `--automount=/host/path`: use the new v1 plugin API to discover plugins that
   will detect this option and select the right containers in which to bind-mount
   volumes. This is really nifty...

Close openedx-unsupported/wg-developer-experience#43
regisb added a commit to overhangio/tutor that referenced this issue Apr 19, 2022
The `--mount` and `--automount` options are available both with `tutor local`
and `tutor dev` commands. They allow users to easily bind-mount containers from
the host to containers. Yes, I know, we already provide that possibility with
the `bindmount` command and the `--volume=/path/` option. But these suffer from
the following drawbacks:

- They are difficult to understand.
- The "bindmount" command name does not make much sense.
- It's not convenient to mount an arbitrary folder from the host to multiple
  containers, such as the many lms/cms containers (web apps, celery workers and
  job runners).

To address this situation, we now recommend to make use of --mount and
--automount options.

1. `--mount=service1[,service2,...]:/host/path:/container/path`: manually mount
   `/host/path` to `/container/path` in container "service1" (and "service2").
2. `--automount=/host/path`: use the new v1 plugin API to discover plugins that
   will detect this option and select the right containers in which to bind-mount
   volumes. This is really nifty...

Close openedx-unsupported/wg-developer-experience#43
@kdmccormick kdmccormick moved this from In Progress to In Review in Tutor DevEnv Adoption (OLD BOARD) Apr 19, 2022
@kdmccormick
Copy link
Collaborator Author

Will be resolved by overhangio/tutor#643

regisb added a commit to overhangio/tutor that referenced this issue Apr 20, 2022
The `--mount` option is available both with `tutor local`
and `tutor dev` commands. It allows users to easily bind-mount containers from
the host to containers. Yes, I know, we already provide that possibility with
the `bindmount` command and the `--volume=/path/` option. But these suffer from
the following drawbacks:

- They are difficult to understand.
- The "bindmount" command name does not make much sense.
- It's not convenient to mount an arbitrary folder from the host to multiple
  containers, such as the many lms/cms containers (web apps, celery workers and
  job runners).

To address this situation, we now recommend to make use of --mount:

1. `--mount=service1[,service2,...]:/host/path:/container/path`: manually mount
   `/host/path` to `/container/path` in container "service1" (and "service2").
2. `--mount=/host/path`: use the new v1 plugin API to discover plugins that
   will detect this option and select the right containers in which to bind-mount
   volumes. This is really nifty...

Close openedx-unsupported/wg-developer-experience#43
regisb added a commit to overhangio/tutor that referenced this issue Apr 20, 2022
The `--mount` option is available both with `tutor local`
and `tutor dev` commands. It allows users to easily bind-mount containers from
the host to containers. Yes, I know, we already provide that possibility with
the `bindmount` command and the `--volume=/path/` option. But these suffer from
the following drawbacks:

- They are difficult to understand.
- The "bindmount" command name does not make much sense.
- It's not convenient to mount an arbitrary folder from the host to multiple
  containers, such as the many lms/cms containers (web apps, celery workers and
  job runners).

To address this situation, we now recommend to make use of --mount:

1. `--mount=service1[,service2,...]:/host/path:/container/path`: manually mount
   `/host/path` to `/container/path` in container "service1" (and "service2").
2. `--mount=/host/path`: use the new v1 plugin API to discover plugins that
   will detect this option and select the right containers in which to bind-mount
   volumes. This is really nifty...

Close openedx-unsupported/wg-developer-experience#43
Repository owner moved this from In Review to Closed in Tutor DevEnv Adoption (OLD BOARD) Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Relates to new features or improvements to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants