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

feat: Add migrate Make command. #32705

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Conversation

dianakhuang
Copy link
Contributor

@dianakhuang dianakhuang commented Jul 11, 2023

To make edx-platform more consistent with other IDAs and to help deprecate more of paver, we are adding the ability to run make migrate in a local environment as a replacement for paver update_db.

openedx-unsupported/devstack#1085

Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks.

  1. Are there any docs where this is needed? I guess most of the Makefile commands are called from somewhere, like the provisioning scripts, and those are documented.

[idea] Could we have a migrate-lms, migrate-cms, and migrate command which calls the former two? Then, could the provisioning script call these Makefile commands? I'm not sure if that is better, but I imagine that the provisioning scripts for the other services just call make migrate, so this would be more consistent. Thoughts?

  1. Does .PHONY need to be updated?

Makefile Show resolved Hide resolved
@dianakhuang dianakhuang force-pushed the diana/add-migrate-command branch 2 times, most recently from 5b5c871 to e253024 Compare July 11, 2023 15:11
@dianakhuang
Copy link
Contributor Author

@robrap made this more in line with the devstack Make command and it should be ready for rereview.

Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Minor fix for .PHONY and looks good.

Regarding the devstack Makefile (non-blocking for this PR):

  • Should we update to call this script?
  • Is there a reason that devstack calls showmigrations for cms and lms, but not for other IDAs. Should that just be dropped? What's its purpose?

Makefile Outdated Show resolved Hide resolved
@dianakhuang dianakhuang force-pushed the diana/add-migrate-command branch from e253024 to d461c4a Compare July 12, 2023 16:11
To make edx-platform more consistent with other IDAs and to
help deprecate more of paver, we are adding the ability to
run `make migrate` in a local environment as a replacement
for `paver update_db`.

openedx-unsupported/devstack#1085
@dianakhuang dianakhuang force-pushed the diana/add-migrate-command branch from d461c4a to 93da3d8 Compare July 12, 2023 16:12
@dianakhuang
Copy link
Contributor Author

Yeah, I was planning on moving the devstack code over to use this Make target instead. I think including showmigrations is a convenience/personal preference thing, but I figured I would keep it.

@robrap
Copy link
Contributor

robrap commented Jul 12, 2023

Yeah, I was planning on moving the devstack code over to use this Make target instead. I think including showmigrations is a convenience/personal preference thing, but I figured I would keep it.

Options are:

  1. Devstack makefile drops call to showmigrations.
  2. Leave showmigrations as-is.
  3. Move showmigrations to edx-platform Makefile as separate commands for consistency.
  4. Move showmigrations to the edx-platform migrate-* commands you just added.

I do not feel strongly, and probably would have chosen option 1, but am curious if you think it is a convenience worth keeping, why you would want that in devstack, but not in edx-platform directly (option 4)?

@dianakhuang
Copy link
Contributor Author

@robrap I did do #4. I added it back in after your last code review.

@dianakhuang dianakhuang merged commit e28bbe9 into master Jul 12, 2023
@dianakhuang dianakhuang deleted the diana/add-migrate-command branch July 12, 2023 16:47
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@robrap
Copy link
Contributor

robrap commented Jul 12, 2023

@robrap I did do #4. I added it back in after your last code review.

Got it. Thanks.

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.

3 participants