feat: Add `_operation` variable #1733

lkubb · 2024-08-13T18:33:36Z

Makes exclude configuration templatable.
Adds an _operation variable to the rendering contexts for exclude and tasks, representing the current operation - either copy~~, recopy~~ or update.
~~Adds the (semi-)undocumented _copier_python, _copier_conf.os and _folder_name variables to the docs in a second commit (since I restructured them anyways)~~ ~~extracted into docs: Restructure context vars, add undocumented ones #1856, will rebase once it's merged~~ rebased

Notes:
This was proposed here: #1718 (comment)

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 _operation (!/=)= "update".

Fixes: #1725
Fixes: #1625

Alternative to #1732

yajo

The docs would need to reflect this change too.

yajo · 2024-08-16T11:15:51Z

copier/main.py

@@ -234,7 +238,7 @@ def _cleanup(self) -> None:
        for method in self._cleanup_hooks:
            method()

-    def _check_unsafe(self, mode: Literal["copy", "update"]) -> None:
+    def _check_unsafe(self, mode: Operation) -> None:


No need to pass ˋmodeˋ if it is in ˋself.operationˋ, right?

Initially I did it like that, but noticed that this would cause a behavior change (at least in theory):

During _apply_update(), self.operation is update, but it calls on run_copy() several times, which would pass copy to _check_unsafe() before this patch.

Yes. However, that is correct. You will notice that there are calls to replace. In those calls, you can replace some configuration for the sub-worker that is created. Could you please try doing it that way?

Sorry, I'm not sure I'm following. First, let me sum up:

Introducing _copier_conf.operation means we have an attribute on the worker representing the current high-level (user-requested) operation.

You're proposing to use this reference for _check_unsafe instead of the parameter.

I noted that doing this will change how _check_unsafe behaves during the individual copy operations that run during an update, where the high-level operation is update, but the low-level one is copy, advocating for keeping the parameter.

I'm already using replace for overriding the operation during update. Are you saying the high-level operation during the individual copy operations should be copy? Because that would mean _copier_conf.operation is always copy during template rendering, i.e. defeat the purpose of this feature.

tests/conftest.py

copier/main.py

codecov · 2024-08-17T08:37:51Z

Codecov Report

Attention: Patch coverage is 98.61111% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.75%. Comparing base (2e7629e) to head (2a80d01).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
copier/types.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1733      +/-   ##
==========================================
- Coverage   97.67%   95.75%   -1.92%     
==========================================
  Files          49       50       +1     
  Lines        5238     5306      +68     
==========================================
- Hits         5116     5081      -35     
- Misses        122      225     +103

Flag	Coverage Δ
unittests	`95.75% <98.61%> (-1.92%)`	⬇️

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.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

lkubb · 2024-09-21T08:13:30Z

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

yajo

Sorry for taking so long to review! I don't have a lot of time and this one required deep thinking.

copier/main.py

yajo · 2024-09-22T07:37:24Z

copier/main.py

@@ -234,7 +238,7 @@ def _cleanup(self) -> None:
        for method in self._cleanup_hooks:
            method()

-    def _check_unsafe(self, mode: Literal["copy", "update"]) -> None:
+    def _check_unsafe(self, mode: Operation) -> None:


Yes. However, that is correct. You will notice that there are calls to replace. In those calls, you can replace some configuration for the sub-worker that is created. Could you please try doing it that way?

copier/main.py

tests/test_context.py

sisp

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

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()). 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.

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

from __future__ import annotations

import sys
from contextvars import ContextVar
from dataclasses import dataclass, replace
from functools import wraps
from typing import Callable, Literal, TypeVar

if sys.version_info >= (3, 10):
    from typing import ParamSpec
else:
    from typing_extensions import ParamSpec


Operation = Literal["copy", "update"]
P = ParamSpec("P")
R = TypeVar("R")

_operation: ContextVar[Operation] = ContextVar("_operation")


def as_operation(value: Operation) -> Callable[[Callable[P, R]], Callable[P, R]]:
    def _decorator(func: Callable[P, R]) -> Callable[P, R]:
        @wraps(func)
        def _wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
            token = _operation.set(_operation.get(value))
            try:
                return func(*args, **kwargs)
            finally:
                _operation.reset(token)

        return _wrapper

    return _decorator


@dataclass
class Worker:
    template: str

    @as_operation("copy")
    def run_copy(self) -> None:
        print(f"Worker({self.template}).run_copy(): {_operation.get()}")

    @as_operation("update")
    def run_update(self) -> None:
        print(f"Worker({self.template}).run_update(): {_operation.get()}")
        replace(self, template="old").run_copy()
        replace(self, template="new").run_copy()


worker = Worker("new")

print("$ copier copy ...")
worker.run_copy()
# -> Worker(new).run_copy(): copy

print("$ copier update ...")
worker.run_update()
# -> Worker(new).run_update(): update
# -> Worker(old).run_copy(): update
# -> Worker(new).run_copy(): update

The Worker.run_copy() and Worker.run_update() methods both set their operation context via the decorator as_operation(). The decorator wraps the decorated method and sets the operation context to the provided value only when the operation context has not been set yet; if the operation context has been set, it remains as is. Thus, a call of Worker.run_update() sets the operation context to "update", and when Worker.run_copy() gets called within this context, the operation context will remain "update".

Using ContextVar is thread-safe. Also, the following requirement stated in the Python docs

Important: Context Variables should be created at the top module level and never in closures. Context objects hold strong references to context variables which prevents context variables from being properly garbage collected.

is met.

I agree that the Jinja variable _copier_conf.operation only seems to make sense when rendering template expressions in copier.yaml. And although the update seems to produce correct results, the fact that the _copier_conf.operation value is unstable during an update (while rendering files etc.), it is asking for trouble IMO. 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.

WDYT, @lkubb @yajo?

lkubb · 2024-11-08T02:11:19Z

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.

sisp

Thanks for updating this PR, @lkubb! 👍 This looks very good, just a couple of quite minor requests and ideas from my side.

copier/main.py

docs/configuring.md

docs/creating.md

tests/test_context.py

mschoettle · 2024-11-11T22:09:06Z

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 · 2024-11-11T22:18:45Z

@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 #1574 (comment)

sisp

Great! Just some absolutely minor remarks. Once #1856 has been merged and this one rebased, I think we can merge. But I'd like to give also @yajo the opportunity to give feedback, as he performed the first review iteration.

copier/main.py

docs/configuring.md

tests/test_context.py

docs/creating.md

sisp

Fantastic work, @lkubb! 👌 LGTM! 🎉

lkubb changed the title ~~Add _copier_conf.operation variable~~ feat: Add _copier_conf.operation variable Aug 13, 2024

lkubb force-pushed the feat/operation-var branch from 2a53803 to eea53da Compare August 13, 2024 18:35

yajo requested changes Aug 16, 2024

View reviewed changes

yajo mentioned this pull request Aug 16, 2024

Add Exclude on Update #1732

Open

lkubb requested a review from yajo August 16, 2024 16:31

lkubb force-pushed the feat/operation-var branch 2 times, most recently from 4ba043b to e0cac30 Compare August 17, 2024 08:31

lkubb mentioned this pull request Aug 27, 2024

[Bug] Boilerplate is regenerated during updates if deleted salt-extensions/salt-extension-copier#41

Closed

yajo requested changes Sep 22, 2024

View reviewed changes

lkubb force-pushed the feat/operation-var branch 3 times, most recently from f16e203 to cf4934a Compare September 26, 2024 10:13

lkubb requested a review from yajo September 26, 2024 10:41

lkubb force-pushed the feat/operation-var branch 3 times, most recently from 735f33e to 1e67a47 Compare October 22, 2024 08:06

sisp reviewed Oct 26, 2024

View reviewed changes

lkubb force-pushed the feat/operation-var branch from 1e67a47 to ab02a1a Compare November 8, 2024 11:21

lkubb changed the title ~~feat: Add _copier_conf.operation variable~~ feat: Add _operation variable Nov 8, 2024

lkubb force-pushed the feat/operation-var branch 4 times, most recently from 819b58b to a55f2c9 Compare November 8, 2024 12:12

lkubb requested a review from sisp November 8, 2024 12:29

sisp requested changes Nov 11, 2024

View reviewed changes

lkubb mentioned this pull request Nov 11, 2024

docs: Restructure context vars, add undocumented ones #1856

Merged

lkubb force-pushed the feat/operation-var branch from 6db03a2 to 126337f Compare November 11, 2024 22:04

lkubb force-pushed the feat/operation-var branch from 126337f to 75c5603 Compare November 11, 2024 22:08

lkubb force-pushed the feat/operation-var branch 2 times, most recently from 204741f to 98746b5 Compare November 12, 2024 00:58

sisp requested changes Nov 12, 2024

View reviewed changes

copier/main.py Show resolved Hide resolved

docs/configuring.md Outdated Show resolved Hide resolved

tests/test_context.py Outdated Show resolved Hide resolved

docs/creating.md Outdated Show resolved Hide resolved

feat: Add _operation variable

2a80d01

lkubb force-pushed the feat/operation-var branch from 7a27c87 to 2a80d01 Compare November 12, 2024 21:06

sisp approved these changes Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `_operation` variable #1733

feat: Add `_operation` variable #1733

lkubb commented Aug 13, 2024 •

edited

Loading

yajo left a comment

yajo Aug 16, 2024

lkubb Aug 16, 2024 •

edited

Loading

yajo Sep 22, 2024

lkubb Sep 22, 2024 •

edited

Loading

codecov bot commented Aug 17, 2024 •

edited

Loading

lkubb commented Sep 21, 2024

yajo left a comment

yajo Sep 22, 2024

sisp left a comment •

edited

Loading

lkubb commented Nov 8, 2024 •

edited

Loading

sisp left a comment

mschoettle commented Nov 11, 2024

lkubb commented Nov 11, 2024 •

edited

Loading

sisp left a comment

sisp left a comment

feat: Add _operation variable #1733

Are you sure you want to change the base?

feat: Add _operation variable #1733

Conversation

lkubb commented Aug 13, 2024 • edited Loading

yajo left a comment

Choose a reason for hiding this comment

yajo Aug 16, 2024

Choose a reason for hiding this comment

lkubb Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

yajo Sep 22, 2024

Choose a reason for hiding this comment

lkubb Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 17, 2024 • edited Loading

Codecov Report

lkubb commented Sep 21, 2024

yajo left a comment

Choose a reason for hiding this comment

yajo Sep 22, 2024

Choose a reason for hiding this comment

sisp left a comment • edited Loading

Choose a reason for hiding this comment

lkubb commented Nov 8, 2024 • edited Loading

sisp left a comment

Choose a reason for hiding this comment

mschoettle commented Nov 11, 2024

lkubb commented Nov 11, 2024 • edited Loading

sisp left a comment

Choose a reason for hiding this comment

sisp left a comment

Choose a reason for hiding this comment

feat: Add `_operation` variable #1733

feat: Add `_operation` variable #1733

lkubb commented Aug 13, 2024 •

edited

Loading

lkubb Aug 16, 2024 •

edited

Loading

lkubb Sep 22, 2024 •

edited

Loading

codecov bot commented Aug 17, 2024 •

edited

Loading

sisp left a comment •

edited

Loading

lkubb commented Nov 8, 2024 •

edited

Loading

lkubb commented Nov 11, 2024 •

edited

Loading