Rethink the shell expected result

[olblak]

I have been involved lately in different discussion about the shell resource. So I am putting here my thought looking for feedbacks.

At the moment, Updatecli shell resource return the following results

Source:

• Success if command return code == 0
• Error if command return code != 0

Condition:

• Success if command return code == 0
• Error if command return code != 0

Target:

• Warning if command return code == 0 && output on the console
• Success if command return code == 0 && no output on the console
• Error if command return code != 0

While this behavior was a good idea to bootstrap the shell resource, I also need to regularly read to documentation each time I need to write a script for Updatecli
Now that the shell resource is used in many different scenarios, I start to have a better understanding of the problem

I can't find a solution that fit all my needs so I am wondering if it's not better to offer the ability to have different ways of mesure success
I propose to add a new key to define the kind of expected result

Using file checksum

targets:
  example:
    kind: shell
    spec:
      command: true
      result:
        kind: file/checksum
          spec:
           files:
            - file1.txt
            - file2.txt
            - file3.txt

=> If file checksum for listed files is the same before and after running the shell command then we return no changed

Checking command return code

targets:
  example:
    kind: shell
    spec:
      command: true
      result:
        kind: exitcode
        spec:
          failure: 1
          changed: 2
          success: 0

=> Result is defined based on command return code

Checking if files exist

targets:
  example:
    kind: shell
    spec:
      command: true
      result:
        kind: file/exist
        spec:
          files:
            - file1.txt
            - file2.txt
           - file3.txt

=> If some of the listed files are created before and after running the shell command then we return something changed.
If files are missing after a run then we assume an error
If the list of files already exist before the shell command then we skip

Current behavior
And the current mode for backward compatibility

Current mode

targets:
  example:
    kind: shell
    spec:
      command: true
      result:
        kind: console/output

I guess we can still add result kind later based on the need.

The purpose of the result is to simplify so we don't have to handle the different results

[olblak]

That being said, maybe I am just over engineering things :D

[olblak]

I think instead of "result" I would prefer "outcome" to express the expectaction of the shell resource

[dduportal]

I like the idea of using different exit code to ensure different outcomes. It reminds me of the Terraform-detailed-exitcode flag.

I can't remember exactly why I did the whole "output" stuff initially and I fear it was only "because it looked easy".

Switching to an exit-code based behavior make sense to me:

• First version would print a "Warning depreciation" when a shell target would return an exit code 0 an output
• Next minor version would keep the warning but would change the behavior
• Then next minor version would change behavior

WDYT?

Checking if files exist

and

Using file checksum

Both feels like over-engineered compared to a shell instruction test -f filename.txt && exit 0 || exit 2 for instance.
I honestly do not see any added value (compared to the amount of YAML to write to specify this) for the "file exist".

---

While experimenting code changes for #465, I stumbled upon this behavior of the shell provider.

Updatecli has to git add the changed files. Removing the concept of SCM from resources to ensure that both apply and diff would have the same behavior, makes the shell target cumbersome.

I thought about a way to solve this differently: instead of asking the shell resource to determine which file was changed (e.g. the user 😅), what if updatecli could do it instead?
The idea was to use git to check the diff-ed file for all resources and let the pipeline processing to take care of it: less pain for the end user (their script would "only" take care of changing the file(s)) and less code complexity for plugins.

But this ain't a free meal to implement compared to the exit code solution you mention here.

[olblak]

I was also thinking to rely on Git but ideally I'd like continuing using Updatecli from none git repositories as well

[olblak]

Thanks a lot for your feedback.

The file checksum and file exist, are example where I envision have different kind of expected outcome to ultimately simply the scripting part.
And example is on the npm PR #1065

Where I have the following manifest

targets:
    npm:
        name: Bump "@mdi/font" package version
        kind: json
        spec:
            file: package.json
            key: dependencies.@mdi/font
        scmid: default
        sourceid: npm
    package-lock.json:
        dependson:
            - npm
        name: Update NPM lockfile package-lock.json
        kind: shell
        spec:
            command: npm install --package-lock-only
        scmid: default
        disablesourceinput: true

And I would like the shell target to detect if the package-lock.json was modified even if the file is not in a git repository
I could put all the logic in the script but I think something like

targets:
    npm:
        name: Bump "@mdi/font" package version
        kind: json
        spec:
            file: package.json
            key: dependencies.@mdi/font
        scmid: default
        sourceid: npm
    package-lock.json:
        dependson:
            - npm
        name: Update NPM lockfile package-lock.json
        kind: shell
        spec:
            command: npm install --package-lock-only
            outcome:
              kind: file/checksum
              spec:
                files:
                  - "package-lock.json"
        scmid: default
        disablesourceinput: true

Would easily run on MacOSx/Windows/Linux assuming that everybody has npm installed

[olblak]

I'll have a look

[olblak]

I had a quick look to the npm package lockfile.
It seems that I only have to update some hash. No transitive dependencies in place so maybe I can remove the requirement on npm/yarn by handling this part directly from Updatecli

[dduportal]

Worst case: rely on NPM to bump the version: npm install <dependency>@<newversion> with a shell resource (that command will update the package-lock.json for you automatically

[olblak]

After some investigation, I think it's still better to use the following workflow where

1. Updatecli using the json resource update the file package.json so we keep the current structure like dependencies, devdependencies,etc.
2. We run the command npm install --package-lock-only. That command only updates the package-lock.json to match the package.json without downloading dependencies to the local file node_module.

[dduportal]

I would not keep that workflow: only changing the JSON does not ensure that your change is valid. NPM could refuse it because of an incompatible dependency (tree cycle, incompatible transitive dependency and many more issues).

Unless there is a specific value to update only the JSON (but I can't see it: if you have to run a command, better to let the dependency management tool do its job fully)?

[olblak]

I would not keep that workflow: only changing the JSON does not ensure that your change is valid. NPM could refuse it because of an incompatible dependency (tree cycle, incompatible transitive dependency and many more issues).

I agree with you and I think we are misunderstanding each others :D.

One of the early check of the npm autodiscovery plugin is to see if there is package.json and a package-lock.json
Then we can have one of the three following situation

Case 1: No lock file

Easiest scenario
We look for each dependencies and devDependencies specified in package.json and we try to update the file using the json resource (no npm command involved). Updatecli is enough

Case 2: Lock file present but npm is not installed npm --version doesn't work

We send a warning saying that we detected a package.json but we can't handle the lock file so we skip that package.json

Case 3: Lock file is present and npm installed

We use the json resource to retrieve the value from the npm dependency and then we run npm install --package-lock-only which only take care of updating the lock file based on the package.json, nothing more
In the case 3 it would be easier to have the shell resource being able to detect if the file package-lock.json changed by using the file checksum because the script would be only one command npm install --package-lock-only and therefor would work both on Linux/Windows(assuming npm installed)/MacOSx assuming npm is installed

A similar behavior is possible for yarn. yarn relies on yarn.lock
If we

[dduportal]

🤔 it feels like you are trying to reinvent what NPM is doing and it feels dangerous since the goal of updatecli is to help developers to keep their dependencies up to date.

I would use either the NPM API or the npm command line because using the JSON provider for updating a package.json feels the same as running SQL commands on the database while the middleware is running 😱

[olblak]

Well you insight makes me realize that my workflow is broken in many ways as I misenderstood how npm install works :D
While I think updating the file package.json manually via updatecli remains ok and has some advantages.

The lock file is where all the complex stuff happen and this is not what I want to re-implement.

I'll revisit my implementation once I have a better picture of the shell result output