Simplify migrations #1510

AdrianFreundQC · 2024-02-08T17:02:10Z

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?

sisp

Thanks for taking the initiative and working on this long-standing feature proposal, @AdrianFreundQC! 🙏

I've done a first review iteration over the main code and docs (not the tests yet). It looks great, just a couple of minor comments so far.

copier/main.py

copier/template.py

docs/configuring.md

codecov · 2024-02-12T14:00:52Z

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 ⚠️
copier/template.py	96.55%	1 Missing ⚠️
tests/helpers.py	75.00%	1 Missing ⚠️

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%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AdrianFreundQC · 2024-02-16T18:20:11Z

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 · 2024-02-16T18:34:52Z

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

docs/configuring.md

AdrianFreundQC · 2024-02-24T09:44:45Z

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.

sisp · 2024-02-24T10:08:40Z

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

I've done a full review this time. The implementation looks good to me. The legacy format test does not ensure identical behavior as before anymore, and I think the new format needs a bit more systematic testing (because it's more powerful/flexible). I think keeping the legacy tests unchanged and adapting and extending a copy thereof for the new format would be better than surgically adding some testing of the new format and mixing it with the legacy format (see my inline comment for more details).

copier/main.py

copier/template.py

docs/configuring.md

tests/demo_legacy_migrations/migrations.py

tests/test_migrations.py

yajo

This definitely looks like the path forward. Thanks for taking care of all this!

copier/main.py

copier/template.py

docs/configuring.md

tests/test_migrations.py

docs/configuring.md

AdrianFreundQC · 2024-04-02T08:50:55Z

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.

sisp · 2024-04-02T08:58:21Z

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

AdrianFreundQC · 2024-04-25T16:23:05Z

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.

sisp · 2024-04-25T16:33:01Z

@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 #1602 (review) 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 · 2024-04-25T16:35:58Z

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

AdrianFreundQC · 2024-04-25T17:56:24Z

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

yajo

Very amazing work, Thanks!

There's just one minor suggestion to enhance docs. Would you like to push it before merging?

docs/configuring.md

sisp

Looks great, @AdrianFreundQC! 👌 Just some totally minor remarks from my side. Plus in general, some lines seem very long. Would you mind running Ruff's formatter once?

About code formatting: IIRC, there is no Nix-based pre-commit hook for Ruff's formatter, which would explain why the flake-check checks pass although I'm sure Ruff's formatter would disagree. @yajo Is there a way to enable Ruff's formatter to check code formatting? And does the Nix-locked version of Ruff even have the formatter already? We might need to update the lock file.

copier/template.py

AdrianFreundQC · 2024-05-02T11:39:36Z

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.

sisp

Sorry for the delay. 🫣

Content-wise it looks good to me. I still see some minor formatting issues. Would you mind trying to fix them?

tests/helpers.py

tests/test_migrations.py

tests/test_tasks.py

tests/test_migrations.py

AdrianFreundQC · 2024-05-15T08:08:44Z

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

sisp

Looks good now! 🎉 Thank you very much for the hard work, implementing this great feature with proper tests, and patience with the reviews even on pedantic formatting fixes! 🙏 Fantastic job! 🥇 Let's get this merged! 🚀

AdrianFreundQC · 2024-06-27T08:42:31Z

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

yajo · 2024-06-28T17:40:19Z

I'd like to at least have #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 · 2024-06-28T17:49:16Z

Ah, nevermind. I just opened #1681. It was an easy one.

sisp mentioned this pull request Feb 9, 2024

WIP, add pre/post copy and pre/post update tasks #1511

Open

sisp requested changes Feb 11, 2024

View reviewed changes

AdrianFreundQC force-pushed the simplify-migrations branch 5 times, most recently from f62dbf5 to aa1c40b Compare February 15, 2024 15:41

AdrianFreundQC requested a review from sisp February 16, 2024 18:18

AdrianFreundQC commented Feb 16, 2024

View reviewed changes

docs/configuring.md Outdated Show resolved Hide resolved

sisp requested changes Feb 26, 2024

View reviewed changes

yajo requested changes Feb 29, 2024

View reviewed changes

hparfr reviewed Mar 11, 2024

View reviewed changes

docs/configuring.md Show resolved Hide resolved

AdrianFreundQC force-pushed the simplify-migrations branch 4 times, most recently from 22d1ce4 to 7d5c7be Compare April 2, 2024 15:18

AdrianFreundQC force-pushed the simplify-migrations branch 4 times, most recently from 131b22c to 9d31db2 Compare April 25, 2024 15:58

AdrianFreundQC force-pushed the simplify-migrations branch 2 times, most recently from de08c15 to 4756477 Compare April 25, 2024 17:51

yajo approved these changes Apr 29, 2024

View reviewed changes

docs/configuring.md Outdated Show resolved Hide resolved

AdrianFreundQC force-pushed the simplify-migrations branch from 4756477 to e31c826 Compare April 30, 2024 06:55

sisp requested changes May 1, 2024

View reviewed changes

copier/template.py Outdated Show resolved Hide resolved

copier/template.py Outdated Show resolved Hide resolved

AdrianFreundQC force-pushed the simplify-migrations branch 2 times, most recently from 0713185 to 1dd1228 Compare May 2, 2024 11:35

AdrianFreundQC requested a review from sisp May 14, 2024 13:19

sisp requested changes May 14, 2024

View reviewed changes

AdrianFreundQC force-pushed the simplify-migrations branch from 1dd1228 to ff188ff Compare May 15, 2024 08:06

freundTech added 3 commits May 15, 2024 10:06

feat: Simplify migration configuration

4611e6a

test: Add test for new task and migration configuration

d71ef45

docs: Document new migration and task configuration

d80c011

AdrianFreundQC force-pushed the simplify-migrations branch from ff188ff to d80c011 Compare May 15, 2024 08:06

sisp approved these changes May 15, 2024

View reviewed changes

sisp merged commit d6f226f into copier-org:master May 15, 2024
21 checks passed

AdrianFreundQC deleted the simplify-migrations branch May 15, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify migrations #1510

Simplify migrations #1510

AdrianFreundQC commented Feb 8, 2024

sisp left a comment

codecov bot commented Feb 12, 2024 •

edited

Loading

AdrianFreundQC commented Feb 16, 2024

AdrianFreundQC commented Feb 16, 2024

AdrianFreundQC commented Feb 24, 2024

sisp commented Feb 24, 2024

sisp left a comment •

edited

Loading

yajo left a comment

AdrianFreundQC commented Apr 2, 2024

sisp commented Apr 2, 2024

AdrianFreundQC commented Apr 25, 2024 •

edited

Loading

sisp commented Apr 25, 2024

sisp commented Apr 25, 2024

AdrianFreundQC commented Apr 25, 2024

yajo left a comment

sisp left a comment •

edited

Loading

AdrianFreundQC commented May 2, 2024

sisp left a comment

AdrianFreundQC commented May 15, 2024

sisp left a comment

AdrianFreundQC commented Jun 27, 2024

yajo commented Jun 28, 2024

yajo commented Jun 28, 2024

Simplify migrations #1510

Simplify migrations #1510

Conversation

AdrianFreundQC commented Feb 8, 2024

Tasks

Migrations

Current State

sisp left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 12, 2024 • edited Loading

Codecov Report

AdrianFreundQC commented Feb 16, 2024

AdrianFreundQC commented Feb 16, 2024

AdrianFreundQC commented Feb 24, 2024

sisp commented Feb 24, 2024

sisp left a comment • edited Loading

Choose a reason for hiding this comment

yajo left a comment

Choose a reason for hiding this comment

AdrianFreundQC commented Apr 2, 2024

sisp commented Apr 2, 2024

AdrianFreundQC commented Apr 25, 2024 • edited Loading

sisp commented Apr 25, 2024

sisp commented Apr 25, 2024

AdrianFreundQC commented Apr 25, 2024

yajo left a comment

Choose a reason for hiding this comment

sisp left a comment • edited Loading

Choose a reason for hiding this comment

AdrianFreundQC commented May 2, 2024

sisp left a comment

Choose a reason for hiding this comment

AdrianFreundQC commented May 15, 2024

sisp left a comment

Choose a reason for hiding this comment

AdrianFreundQC commented Jun 27, 2024

yajo commented Jun 28, 2024

yajo commented Jun 28, 2024

codecov bot commented Feb 12, 2024 •

edited

Loading

sisp left a comment •

edited

Loading

AdrianFreundQC commented Apr 25, 2024 •

edited

Loading

sisp left a comment •

edited

Loading