feat: Add `_copier_conf.operation` variable

[lkubb]

Adds operation to _copier_conf, representing the current operation - either copy, recopy or update. This was proposed here: https://github.com/copier-org/copier/issues/1718#issuecomment-2282643624

I hope the way it's implemented and tested is as intended by @yajo. ~~The tests are quite synthetic,~~ I would expect the usual application to be _copier_conf.operation (!/=)= "update".

Fixes: https://github.com/copier-org/copier/issues/1725

Alternative to https://github.com/copier-org/copier/pull/1732

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.02%. Comparing base (ecbe5fa) to head (dced5eb). Report is 121 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1733      +/-   ##
==========================================
+ Coverage   98.00%   98.02%   +0.01%     
==========================================
  Files          53       54       +1     
  Lines        5720     5775      +55     
==========================================
+ Hits         5606     5661      +55     
  Misses        114      114              

Flag	Coverage Δ
unittests	98.02% <100.00%> (+0.01%)	:arrow_up:

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.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lkubb]

@yajo Is there anything I still need to do here? Just making sure you noticed the changes. :)

[lkubb]

Sorry for taking so long to give feedback. 🫣 This PR really needs some deep thinking.

@sisp Thank you for your thoughtful feedback and sorry for taking my time to reply as well.

Adding the attribute operation to the Worker class seems conceptually wrong to me because we'd make the operation mode part of its state while it also has methods to execute either operation (run_copy() and run_update()).

Agreed, I wasn't really happy with it myself, also because it meant the forced inclusion of the operation in inappropriate contexts. Part of this design was caused by the tight coupling of the Copier worker with the renderer imho, I wasn't sure how to solve it properly without a major refactoring.

Thus, it would be possible to set the operation attribute to "update" and call run_copy() (or set the operation attribute to "copy" and call run_update()) which would lead to incorrect behavior. This indicates a bad design to me.

Note that it's only the former situation that leads to incorrect behavior, the latter is the expected path since the run_update method replaces the operation value itself.

How about using a context variable to manage the operation context? This is a simplified example of what I mean:

Thanks a lot for your well thought-out proposal. I agree this design is superior and am currently implementing it. I'm still contemplating a detail since it aggravates a flaw I just noticed in the exclude rendering logic by decoupling the context switch implementation from the worker:

Since match_excludes is a cached property whose value now depends on the _operation, a changed _operation is not reflected in its value if an instance has already been used in a different context. This is not an issue in practice currently since a context switch always coincides with the creation of a new worker instance via replace, but it's definitely a gotcha in case this changes at some point.

[Note: An analog of this is currently present in match_skip, especially if externally mutated instance attributes need to be considered in worker methods].

I might need to address this by deleting the property's cached value explicitly in run_(re)copy/run_update to force regeneration, which is still a bit smelly, or turning it into a cached function instead (less smelly, but a breaking change).

So, how about omitting the _copier_conf.operation variable in the Jinja context while rendering files, directories, and path names? We'd need to document this behavior of course.

👍 Your proposal allows to restrict it to the scopes where it makes sense, namely exclude and task templating.

[mschoettle]

Adds an _operation variable to the rendering contexts for exclude and tasks, representing the current operation - either copy, recopy or update.

What this PR adds is exactly what I am missing.

I recently started using copier for an infrastructure project using compose, env files etc. with various different configuration options.

There are some things, such as questions/answers and tasks, that I don’t want to ask/run on update. And, other tasks should only run when an update is performed.

I am wondering what the rationale is behind not making the operation available to questions (conditions), if I read the above correctly. Would it be possible to use it in when?

[lkubb]

@mschoettle

There are some things, such as questions/answers and tasks, that I don’t want to ask/run on update. And, other tasks should only run when an update is performed.

For the latter I'd recommend using migrations. They can be configured to run independent of the involved versions.

I am wondering what the rationale is behind not making the operation available to questions (conditions), if I read the above correctly. Would it be possible to use it in when?

No, the reason is that when == False variables are not saved, so you'd lose previously provided answers during an update. If I understood your requirements correctly, there's a workaround described in https://github.com/copier-org/copier/issues/1574#issuecomment-2067980359

[pawamoy]

Do we agree that this PR also fixes the long-standing https://github.com/copier-org/copier/issues/240?

[sisp]

@pawamoy I think it addresses part of #240, but pre/post copy/update granularity is not addressed. There's #1511, but it seems stale.

@lkubb @copier-org/maintainers A thought that originates from https://github.com/copier-org/copier/pull/1948#pullrequestreview-2578227878 (and subsequent comments): I think we should rename _operation to _copier_operation.

[pawamoy]

If I read #240 correctly, pre/post were specific to migrations, and are now handled with a _stage variable being "before" or "after". Tasks get the same _stage variable with value "task".

I'm not sure what pre-copy, post-copy, pre-update, or post-update would mean exactly. IMO this PR solves #240.

Let me also link again to my comment above about a third update-diff operation, which I think is super important to have.

[sisp]

You're right, I forgot about the availability of _stage now. :facepalm: Yes, this PR solves #240 then. :+1:

(I've also replied to your comment above.)

[pawamoy]

Actually after seeing a few duplicates of #240 like https://github.com/copier-org/copier/issues/635, I think this PR only partially solves it, as you pointed @sisp. Users want to run things before and after a copy, or before and after an update. The operation won't allow that. Currently tasks only run after. Using context hooks to run things before would be a hack (earliest is when the first Jinja template string is rendered, which is definitely not before anything). But anyway we could close #240 and re-open a fresh issue to get rid of all the legacy discussions about it.

[sisp]

Ah, you're right, @pawamoy. I got confused. :confused: But looking at https://copier.readthedocs.io/en/stable/updating/#how-the-update-works (and thinking about the detailed implementation), I think pre-/post-migrations are equivalent to pre-/post-update tasks for copier update – although implementing pre-/post-update tasks that run always (independent of template versions) is a bit hacky. So the only thing we seem to be missing is a pre-copy task. Right? :thinking:

[pawamoy]

So the only thing we seem to be missing is a pre-copy task. Right?

Seems like it, yes! I feel like this deserves a proper entry in the docs, something like "Pre/post hooks", which is how Cookiecutter named them, and will be familiar to most users. This entry would document that you can use migration as pre/post-update hooks (with examples), tasks as post-copy or post-update hooks, and... pre-copy hooks are not supported yet. The docs entry should emphasize that for simple message display, users can set message_after_copy and related settings. Let me copy this in a new docs issue.

[lkubb]

I think we should rename _operation to _copier_operation.

@sisp Done and rebased.

Edit: Having read the discussion in https://github.com/copier-org/copier/pull/1948, I want to emphasize that (because of https://github.com/copier-org/copier/pull/1733#discussion_r1770047960) this patch does NOT make _operation/_copier_operation available in all rendering contexts, it's limited to tasks and exclude. Maybe this aspect should be reconsidered in light of extensions.

Given the many comments this PR has accumulated over the half year it has been open, I'll try to give a final summary of what it provides and where it fits in the Copier hook system.

This PR

1. Adds templating functionality to _exclude items
2. Adds a _copier_operation context variable for the _tasks and _exclude rendering contexts, which indicates whether copier (re)copy or copier update was invoked by the user. It's absent in regular templates.

(1+2) allows to provide dumb boilerplate files which will be excluded during updates (2) additionally relates to hooks, see after a copy below

If you need your logic to run...

before/after any `update`

Use a `migration` without specifying a version constraint and with appropriate `when` (or even `when: 'true'` to run both before and after). Example:

_migrations:
  - this_script_runs_after_all_updates.sh
  - command: this_script_runs_before_all_updates.sh
    when: "{{ _stage == 'before' }}"
  - command: this_script_runs_before_and_after_all_updates.sh
    when: "true"

You can rely on the provided env vars (STAGE/VERSION_{,PEP440_}{FROM,TO,CURRENT} to target specific commands.

before a `copy`

Come up with an ugly hack possibly involving a custom extension or provide a patch to core, not officially supported atm.

after a `copy`

Use a `task` with `when: '{{ _copier_operation == "copy" }}'`. Example:

_tasks:
  - command: this_script_runs_after_copy_operations_only.sh
    when: "{{ _copier_operation == 'copy' }}"

[!NOTE] All of the hook suggestions apply to the generated project only, not the two intermediate copies involved in update operations. If you need a task to run on these intermediate copies as well, specify a plain task. Off the top of my head, I can't come up with any use case for this, it seems fragile.

Is there anything I can do to help carry this PR over the line? It has been stalled for a while now and the release of Copier 9.5.0 broke the ugly hack I was using to simulate _copier_operation:

_tasks:
  # Don't run this when updating.
  # This breaks when recopying.
  - >-
    {%- if
      not _copier_conf.answers.last
      and '.new_copy.' not in _copier_conf.dst_path.name
      and '.old_copy.' not in _copier_conf.dst_path.name -%}
        "{{ _copier_python }}" "{{ "{src}{sep}tasks{sep}initialize.py".format(src=_copier_conf.src_path, sep=_copier_conf.sep) }}" init
    {%- endif -%}

Specifically https://github.com/copier-org/copier/pull/1880, which added this line and thereby removed _copier_conf.answers.last from the context (whose value being empty was used as a proxy for a copy operation):

https://github.com/copier-org/copier/blob/e444514e5d714a318cf434bcb7fec13d21dfc93c/copier/main.py#L382