feat: Add `_copier_conf.operation` variable #1733

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

Adds operation to _copier_conf, representing the current operation - either copy, recopy or update. 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 _copier_conf.operation (!/=)= "update".

Fixes: #1725

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

All modified and coverable lines are covered by tests ✅

Project coverage is 97.75%. Comparing base (5ac93ee) to head (80e9227).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1733      +/-   ##
==========================================
+ Coverage   97.61%   97.75%   +0.13%     
==========================================
  Files          49       50       +1     
  Lines        5042     5084      +42     
==========================================
+ Hits         4922     4970      +48     
+ Misses        120      114       -6

Flag	Coverage Δ
unittests	`97.75% <100.00%> (+0.13%)`	⬆️

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.

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.

yajo · 2024-09-22T07:34:04Z

copier/main.py

@@ -68,6 +68,9 @@
 _T = TypeVar("_T")


+Operation = Literal["copy", "update"]


Please move this to copier.types module

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?

yajo · 2024-09-22T07:43:02Z

copier/main.py

@@ -203,6 +206,7 @@ class Worker:
    unsafe: bool = False
    skip_answered: bool = False
    skip_tasks: bool = False
+    operation: Operation = "copy"


thought: I find this a bit counter-intuitive. We have to pass an operation now to the worker. The operation could be something like "update" for example. But then we can run_copy() or run_update().

It seems like duplicated features and inconsistent API behavior. If this is the case now, I think we should have a unified run_operation() method that runs either one or the other behind the scenes.

When the workers are created, they should be with the correct operation, and from there onwards use that unless replace() changes it.

We have to pass an operation now to the worker.

We mustn't pass it to the worker though, it's an internal reference that has to be copy by default, only overridden by calls to replace in run_update.

I wanted to declare this field as init=False for that reason, but that does not work together with replace by default (see the last paragraph on dataclasses.replace). This can be fixed by using a custom replace function though.

yajo · 2024-09-22T07:46:42Z

tests/test_context.py

+        git_save()
+    getattr(copier, f"run_{operation}")(str(dst), overwrite=True)
+    expected = "copy" if operation == "recopy" else operation
+    assert ctx_file.read_text() == expected


thought: So, let's say that we copy one template and render the value. The file will contain copy.

Now we run it again. The file contains update. The change is OK because the replay renders copy and detects the change.

However, what would happen if we run it yet again? The replay will render copy, but in reality it is update. Now the next one is also update.

Triple-thinking this... are we introducing here a way to make replays non-reproducible and thus maybe making the update algorithm unreliable? Because there's no way Copier could know if last play was a copy or an update.

CC @copier-org/maintainers. Should we just reject this feature to avoid shooting our own feet?

Disregarding that _copier_conf.operation is not intended to be used for template content changes, this test for checking the value of operation only works because the file containing {{ _copier_conf.operation }}

is deleted

is marked as skip_if_exists, which means it will be regenerated during update if deleted.

What would happen without it being deleted is the following:

copier copy renders the template, it contains copy

copier recopy overwrites the template, it contains copy

copier update updates the template, it still contains copy. This is valid for each subsequent update.

Why (3):
During _apply_update, old_copy and new_copy are both rendered with _copier_conf.operation == "update". The file contains copy though, which is detected as a custom change and reapplied.

_copier_conf.operation should be documented as a meta variable, intended for templating copier.yml configuration. It cannot be used to influence template contents (reliably).

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 added 2 commits August 17, 2024 10:28

feat: Add _copier_conf.operation variable

250820f

Refactor tests, render Jinja in _exclude

f010d84

lkubb force-pushed the feat/operation-var branch from 031e199 to 54fc0b7 Compare August 17, 2024 08:29

Document _copier_conf.operation

e0cac30

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

Make recopy == copy

80e9227

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `_copier_conf.operation` variable #1733

feat: Add `_copier_conf.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

yajo Sep 22, 2024

yajo Sep 22, 2024

lkubb Sep 22, 2024

yajo Sep 22, 2024

lkubb Sep 22, 2024 •

edited

Loading

		@@ -68,6 +68,9 @@
		_T = TypeVar("_T")


		Operation = Literal["copy", "update"]

feat: Add _copier_conf.operation variable #1733

Are you sure you want to change the base?

feat: Add _copier_conf.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

yajo Sep 22, 2024

Choose a reason for hiding this comment

yajo Sep 22, 2024

Choose a reason for hiding this comment

lkubb Sep 22, 2024

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

feat: Add `_copier_conf.operation` variable #1733

feat: Add `_copier_conf.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

lkubb Sep 22, 2024 •

edited

Loading