add diff_filter param to source config [closing #47]

[fredwangwang]

47

expose diff-filter to alter the behavior of check directly. Feels adding something like check_config is going to introduce a level of unnecessary abstraction.

[fredwangwang]

Hi @vito, I tried to change the options to check-filter, which allows filtering based on specific out parameter (e.g. release). However, it is hard to find a easy one to one mapping between underlying git log --diff-filterand check-filter.

For example: add could be translated to git log --diff-filter A -- $pool_name/unclaimed, except it is wrong. The above expression can actually capture both add and release. When a resource is released, the underlying git operation is: Rename $pool_name/claimed -> $pool_name/unclaimed. But from the perspective of $pool_name/unclaimed, it treates the resource as it is added to the folder without needing to know where it comes from, resulting the above git log captures release as well.

So the correct operation to check add is (pseudo-code):

(git log --diff-filter A -- $pool_name/unclaimed).filter(x -> x not in (git log --diff-filter R -- $pool_name))[0]

Here is a table describe all the operations that I believe are correct:

Check Filter	Corresponding underlying operation (pseudo-code)
acquire $name	(git log --diff-filter A -- $pool_name/claimed).filter(x -> x in (git log --diff-filter R -- $pool_name)).filter(x -> $name in x.changed_files)
claim	(git log --diff-filter A -- $pool_name/claimed).filter(x -> x in (git log --diff-filter R -- $pool_name))
release	(git log --diff-filter A -- $pool_name/unclaimed).filter(x -> x in (git log --diff-filter R -- $pool_name))
add	(git log --diff-filter A -- $pool_name/unclaimed).filter(x -> x not in (git log --diff-filter R -- $pool_name))
add_claimed	(git log --diff-filter A -- $pool_name/claimed).filter(x -> x not in (git log --diff-filter R -- $pool_name))
remove	git log --diff-filter D -- $pool_name
update	git log --diff-filter M -- $pool_name

So, while it is doable, it is definitely more than some simple bash which I know of. And it adds additional complexity/maintaince cost in the translation layer. Given the above conclusions, not sure if it is still wroth to expose a higher level configuration.

[fredwangwang]

Added the following description to the readme:

---

Here is a mapping between diff-filter and check behavior:

Diff-filter	Check Behavior
A	add, add_claimed
R	acquire, claim, release
D	remove
M	update

For example, if diff_filter: AM is specificed, it will only check when a resource is added to the pool or the existing resource is updated.

---

Even the low level operation is exposed, hopefully the user can easily chose which diff-filter to apply given the mapping between the filter and the actual checking behavior.

[vito]

@fredwangwang Sorry for the super slow turnaround on this. Now that we're dedicating half of each day to PR review that should get a lot better.

If you're still interested in working on this, I still think it'd be better to have high level parameters, and it'd be worthwhile to add tests too, which would make it easier to verify that the now-more-complicated behavior works properly. But if you'd rather just let this fade away due to the slow turnaround, I wouldn't blame ya.

Broadly speaking, I think the pool resource has always been a bit of a black sheep, and I wonder if there is a better way to express what folks are trying to do with it. It kind of feels like there may be a lot of overlap with the idea of a 'deployment', which came up here: https://github.com/concourse/concourse/discussions/4949#discussioncomment-5667

In this scenario, for example, it seems like you'd maybe have jobs downstream of deployments that trigger when the state of the deployment changes. 🤔

[vito]

Closing due to inactivity but happy to review again if you find the time! Sorry again for the slow initial response time.