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

Needless call to seed_permissions_roles management command when provisioning demo course #1129

Closed
timmc-edx opened this issue Jul 24, 2023 · 3 comments
Assignees

Comments

@timmc-edx
Copy link
Contributor

timmc-edx commented Jul 24, 2023

Devstack and the configuration repo both have scripts to provision a demo course. They first call the import management command (in cms contentstore), and then seed_permissions_roles (in lms discussions). However, the import command already performs the seeding:

https://github.com/openedx/edx-platform/blob/77cef4cd074ca03d2b73261674923eb96ac804bd/cms/djangoapps/contentstore/management/commands/import.py#L78

We can remove this extra management command call from several places:

Archaeological notes:

  • The instance in devstack appears to have been a more recent one, copied from elsewhere in late 2022. The oldest change I can find that adds the call is from 2013, in the configuration repo.
  • But then in 2014, the import command was changed to seed anyhow, which is a more complete fix. I think this means we can drop the explicit call to the seed management command.
@robrap
Copy link
Contributor

robrap commented Aug 1, 2023

We can ask TNL to review the PR.

timmc-edx added a commit that referenced this issue Aug 1, 2023
The `import` management command (from cms contentstore) is called right
before the `seed_permissions_roles` command (from lms discussions).
However, the import command already performs the seeding.

See #1129 for archaeological
notes.

Confirmation that the call doesn't add anything:

- Provisioned LMS with `DEVSTACK_SKIP_DEMO=true`
- Manually ran first two commands for importing demo course (git clone,
  `import` management command)
- Took MySQL DB dumps
- Ran final demo course command, `seed_permissions_roles`
- Took new DB dumps
- Diff of SQL files showed only a change in the dump dates
timmc-edx added a commit that referenced this issue Aug 1, 2023
…1142)

The `import` management command (from cms contentstore) is called right
before the `seed_permissions_roles` command (from lms discussions).
However, the import command already performs the seeding.

See #1129 for archaeological
notes.

Confirmation that the call doesn't add anything:

- Provisioned LMS with `DEVSTACK_SKIP_DEMO=true`
- Manually ran first two commands for importing demo course (git clone,
  `import` management command)
- Took MySQL DB dumps
- Ran final demo course command, `seed_permissions_roles`
- Took new DB dumps
- Diff of SQL files showed only a change in the dump dates
@timmc-edx timmc-edx self-assigned this Aug 2, 2023
@timmc-edx
Copy link
Contributor Author

For remaining occurrences in configuration repo, maybe make a ticket to try deleting the surrounding code entirely, since we're probably not using that at all. (Roles, Jenkins stuff.)

timmc-edx added a commit to openedx-unsupported/configuration that referenced this issue Aug 2, 2023
…rums

This is now redundant with the earlier `import` call, which at this point
goes ahead and does the seeding.

One of these is probably used by sandboxes, and the other is probably not,
but both are referenced by `ansible-provision.sh`. Might as well remove
from both so that at least they're consistent with devstack, and so that
this is not propagated forward to whatever eventually replaces the sandbox
setup scripts.

See openedx-unsupported/devstack#1129 for details.
@timmc-edx
Copy link
Contributor Author

It looks like the other occurrences may still be in use. Removing both: openedx-unsupported/configuration#6974

timmc-edx added a commit to openedx-unsupported/configuration that referenced this issue Aug 3, 2023
…rums (#6974)

This is now redundant with the earlier `import` call, which at this point
goes ahead and does the seeding.

One of these is probably used by sandboxes, and the other is probably not,
but both are referenced by `ansible-provision.sh`. Might as well remove
from both so that at least they're consistent with devstack, and so that
this is not propagated forward to whatever eventually replaces the sandbox
setup scripts.

See openedx-unsupported/devstack#1129 for details.
jdmulloy pushed a commit to openedx-unsupported/configuration that referenced this issue Sep 14, 2023
…rums (#6974)

This is now redundant with the earlier `import` call, which at this point
goes ahead and does the seeding.

One of these is probably used by sandboxes, and the other is probably not,
but both are referenced by `ansible-provision.sh`. Might as well remove
from both so that at least they're consistent with devstack, and so that
this is not propagated forward to whatever eventually replaces the sandbox
setup scripts.

See openedx-unsupported/devstack#1129 for details.
nsprenkle pushed a commit that referenced this issue Nov 21, 2023
…1142)

The `import` management command (from cms contentstore) is called right
before the `seed_permissions_roles` command (from lms discussions).
However, the import command already performs the seeding.

See #1129 for archaeological
notes.

Confirmation that the call doesn't add anything:

- Provisioned LMS with `DEVSTACK_SKIP_DEMO=true`
- Manually ran first two commands for importing demo course (git clone,
  `import` management command)
- Took MySQL DB dumps
- Ran final demo course command, `seed_permissions_roles`
- Took new DB dumps
- Diff of SQL files showed only a change in the dump dates
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants