oscope-dev / scope

Scoping user machines
https://oscope-dev.github.io/scope/
BSD 3-Clause "New" or "Revised" License
6 stars 3 forks source link

check paths behavior is unintuitive when file doesn't exist #152

Closed rubberduck203 closed 1 day ago

rubberduck203 commented 2 weeks ago

Consider the following doctor group config:

apiVersion: scope.github.com/v1alpha
kind: ScopeDoctorGroup
metadata:
  name: app
  description: Application setup
spec:
  actions:
    - name: poetry-install
      check:
        paths:
          - poetry.lock
      fix:
        commands:
          - poetry install

I expected running scope doctor run to detect that the poetry.lock didn't exist and the fix to be run, but the check passed.

I worked around this by also including a command to explicitly test for the existence of poetry.lock.

apiVersion: scope.github.com/v1alpha
kind: ScopeDoctorGroup
metadata:
  name: app
  description: Application setup
spec:
  actions:
    - name: poetry-install
      check:
        commands:
          - test -f poetry.lock
        paths:
          - poetry.lock
      fix:
        commands:
          - poetry install

This works, but also will trigger the fix to run again the next time because now the poetry.lock file is newer than the last run.
That's not a big deal, because running it a second time is a nop in this particular case, but it did feel unintuitive to me.

If this is the intended behavior, then perhaps the "fix" is to add some documentation and an example.
I wanted to start a dialogue about it though.

noizwaves commented 2 weeks ago

Hey @rubberduck203 , thanks for starting this conversation. How often do we expect there to not be a lock file present? Also, is the problem specific to poetry, or was this used as an example?

I'm wondering if the solution here is to commit the poetry.lock file to source control and sidestep this edge case?

rubberduck203 commented 2 weeks ago

It was just an example @noizwaves.
This issue is about the unintuitive behavior.

If I configure it to check a path, I would expect the path not existing to fail the check.

I don't believe it's an edge case at all.
Ensuring certain files exist seems like scope's bread and butter to me.

rubberduck203 commented 2 weeks ago

I believe the behavior is being caused here.

https://github.com/oscope-dev/scope/blob/851ec567c4a1c54f96a9bb1bc36ff6081ebfa30f/scope/src/doctor/check.rs#L547

When the file doesn't exist, there are no results from the glob and the code doesn't execute.
Behavior could be changed by checking to see if the iterable is empty.
(If we land on the existing behavior being undesirable, which I believe it is.)

noizwaves commented 1 week ago

Ok, after digging in, I agree with it being unintuitive. Just put up https://github.com/oscope-dev/scope/pull/155 which should resolve this.

rubberduck203 commented 1 day ago

Fixed by #155