copier icon indicating copy to clipboard operation
copier copied to clipboard

Simplify migrations

Open AdrianFreundQC opened this issue 1 year ago • 12 comments

Resolves #326

Here is a suggestion for how to simplify migrations. Some changes made to migrations were also applied to tasks for consistency. The task configuration is 100% backwards compatible. The migration configuration is not backwards compatible. The old style should be seen as deprecated. This PR support both old and new style and shows a warning when the old style is used.

Tasks

Tasks can still be configured like before:

_tasks:
  - git init

Additionally Tasks can be a dictionary, which allows passing one or more of the optional arguments when and working_directory. working_directory defaults to the destination directory and can be either relative to it, or absolute. Both arguments can be templated.

_tasks:
  - command: git init
    working_directory: "submodule"
    when: "{{ enable_submodules }}"

Migrations

It is no longer necessary to duplicate the same command line for every version to run a migration script.

_migrations:
  - "{{ _copier_python }} {{ _copier_conf.src_path }}/migrate.py"

This runs twice (before and after) on every update and still passes environment variables to the script. Note that it can't pass $VERSION_(PEP440_)CURRENT because there is no current version. Only a from and to version.

It is still possible to only run migrations on a specific version:

_migrations:
  - version: v1.4
    command: rm old_file.txt || true

In this case $VERSION_(PEP440_)CURRENT is still passed to the command.

This still runs both before and after. working_directory and when are also supported for migrations. In addition all the environment variables passed to the command are also available for use in templating:

_migrations:
  - version: v1.4
    command: rm old_file.txt
    when: "{{ _stage == 'before' }}"

Current State

All the features described here work. I still want to do a bit of code cleanup and improve the documentation in the next few days. I would also like some feedback on this design. Is there anything you don't agree with? Something I should change?

AdrianFreundQC avatar Feb 08 '24 17:02 AdrianFreundQC

Codecov Report

Attention: Patch coverage is 99.06103% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 97.38%. Comparing base (7c174a9) to head (d80c011).

Files Patch % Lines
copier/main.py 90.47% 2 Missing :warning:
copier/template.py 96.55% 1 Missing :warning:
tests/helpers.py 75.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1510      +/-   ##
==========================================
- Coverage   97.39%   97.38%   -0.02%     
==========================================
  Files          48       49       +1     
  Lines        4728     5004     +276     
==========================================
+ Hits         4605     4873     +268     
- Misses        123      131       +8     
Flag Coverage Δ
unittests 97.38% <99.06%> (-0.02%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 12 '24 14:02 codecov[bot]

I pushed the fixes as new commits to simplify review. I can rebase them back into three commits (feat, test, docs) or you can just squash merge if you prefer that.

AdrianFreundQC avatar Feb 16 '24 18:02 AdrianFreundQC

CI failure looks unrelated. Should be fixed with a re-run

AdrianFreundQC avatar Feb 16 '24 18:02 AdrianFreundQC

One more thought I had: the migration command running twice on every update might be confusing and unexpected for new users, however it is required for more complex migrations.

Changing the default value for when from true to {{ _state == "after" }} might solve this issue. Migrations without when only run once, while experience users can still set when to true to have them run in the before and after stage.

AdrianFreundQC avatar Feb 24 '24 09:02 AdrianFreundQC

I like this idea, it sounds like a better default behavior than running the command twice. 👌

By the way, I haven't forgotten about reviewing this PR, just haven't had enough time yet. 🙁 I'll try to do it on Monday. 🤞

sisp avatar Feb 24 '24 10:02 sisp

Sorry for the delay. I've been quite busy recently. I'm still working on the tests for the new migrations implementation. I'll have some time in the next two weeks and if that's not enough I'll probably be able to finish it in the first half of May.

AdrianFreundQC avatar Apr 02 '24 08:04 AdrianFreundQC

Thanks for the update and your continuous work on this PR, @AdrianFreundQC! :pray:

sisp avatar Apr 02 '24 08:04 sisp

I have no idea why the MacOS flake test is suddenly failing now. Maybe a broken cache?

/Users/runner/work/_temp/be5ceacc-d596-4a1f-b513-54880e12e09d.sh: /Users/runner/work/copier/copier/.venv/bin/copier: /Users/runner/work/copier/copier/.venv/bin/python: bad interpreter: No such file or directory

The flake check runs fine on my machine. Also all the other tests work now.

If you want you can already review the new tests. I'll go through all the discussions again and see if there's anything else I still have to do.

AdrianFreundQC avatar Apr 25 '24 16:04 AdrianFreundQC

@AdrianFreundQC Sorry for the inconvenience. It's because macos-latest on GitHub-hosted runners got bumped to macOS 14 (ARM) for which there are no binaries for Python 3.8 and 3.9. Before, macos-latest was referring to macOS 12 (x64) for which those binaries existed. See https://github.com/copier-org/copier/pull/1602#pullrequestreview-2019446447 for more details.

We're waiting for @yajo to resolve #1604 (because of a project-level settings change only he can make). Then, after rebasing your PR, CI checks should work again.

sisp avatar Apr 25 '24 16:04 sisp

One sec, your Python 3.8 and 3.9 CI checks are passing. :thinking: It seems GitHub has added those missing binaries. Let me get back to you.

sisp avatar Apr 25 '24 16:04 sisp

I think all discussions should have been addressed now and the CI seems to be working (assuming my last change didn't break anything).

AdrianFreundQC avatar Apr 25 '24 17:04 AdrianFreundQC

I ran addressed the two comments above and ran ruff format (nix pinned version) on main.py and template.py. I didn't run it on all files to not clutter this PR.

I also rebased on master again.

AdrianFreundQC avatar May 02 '24 11:05 AdrianFreundQC

No problem. I reformatted the files and rebased on master again.

AdrianFreundQC avatar May 15 '24 08:05 AdrianFreundQC

Do you have an ETA on when the next version might get released?

AdrianFreundQC avatar Jun 27 '24 08:06 AdrianFreundQC

I'd like to at least have https://github.com/copier-org/copier/issues/1598 fixed before release. It appeared because of this merge IIRC. It should be quite easy to fix though, I just had very little time lately. Once that one's fixed I'll push the new release. Would you want to contribute it?

yajo avatar Jun 28 '24 17:06 yajo

Ah, nevermind. I just opened https://github.com/copier-org/copier/pull/1681. It was an easy one.

yajo avatar Jun 28 '24 17:06 yajo