Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to define changedif behavior for the shell resource #1071

Merged
merged 32 commits into from
Jan 26, 2023

Conversation

olblak
Copy link
Member

@olblak olblak commented Jan 5, 2023

Opening for visibility

This pullrequest allows to have different success criteria for the shell resource.
It's an initial implementation based on this discussion
I am introduce three new success criteria which are:

  • console/output - (current shell behavior)
  • exitcode where we can specify which exit code to map to.
  • file/checksum: that monitor file checksum to determine success

While the "console/output" introduce no change as it requires no configuration, we can use the following snippet to customize the exit code

sources
  echo:  
    name: Should be succeeding
    kind: shell
    spec:
      command: "echo 1.0.0"
      changedif:
        kind: exitcode
        spec:
          warning: 2
          failure: 1
          success: 0
conditions:
  success:
    disablesourceinput: true
    name: Test the condition succeed according success criteria despite command failing
    kind: shell
    spec:
      command: "false"
      changedif:
        kind: file/checksum
        spec:
          files:
            - e2e/updatecli.d/success.d/shell/checksum.yaml

Test

To test this pull request, you can run the following commands:

cp pkg/plugins/resources/shell/success/console/
go test 
pkg/plugins/resources/shell/success/exitcode/
go test
go build -o bin/updatecli .
./bin/updatecli diff --config e2e/updatecli.d/success.d/command.yaml
./bin/updatecli diff --config e2e/updatecli.d/success.d/shell

Additional Information

Tradeoff

I must admit that I am not a big fan of the word "outcome" another one that I could think of is "successcriteria" but I find it a bit long...

EDIT
I decided to go with success as I think it's pretty self explanatory
EDIT
Finally I leaning toward changedif

Potential improvement

As suggested in the discussion, I could imagine introducing more possible outcome rules such as file/exist when we need to monitor a lot of files

@olblak olblak added enhancement New feature or request resource-shell Resource of kind Shell labels Jan 5, 2023
@olblak olblak marked this pull request as draft January 5, 2023 22:59
olblak and others added 4 commits January 6, 2023 00:12
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Jan 7, 2023

I think instead of outcome, 'success' would better capture the meaning

olblak and others added 7 commits January 10, 2023 08:35
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak olblak marked this pull request as ready for review January 12, 2023 12:12
@olblak
Copy link
Member Author

olblak commented Jan 18, 2023

Damien suggestion would be to replace success to changedif

@olblak olblak added this to the 0.45.0 milestone Jan 23, 2023
@olblak olblak changed the title Shell success criteria Allow to define changedif behavior for the shell resource Jan 24, 2023
@olblak olblak mentioned this pull request Jan 24, 2023
2 tasks
@dduportal
Copy link
Contributor

Damien suggestion would be to replace success to changedif

changed_if to be precise :)

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Jan 24, 2023

Damien suggestion would be to replace success to changedif

changed_if to be precise :)

After some experimentation with the jsonschema changed_if sounds fine so I'll proceed with it

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Jan 25, 2023

For a reason that I ignore the following line

ChangedIf SpecChangedIf `yaml:"changed_if,omitempty" json:"changed_if,omitempty"`

Do no extract the changed_if but changedif, without the lowercase

@olblak
Copy link
Member Author

olblak commented Jan 25, 2023

But the json tag works correctly as the generated jsonschema has the property changed_if

Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Jan 25, 2023

Even thought I prefer the lowercase syntax, I can't figure out why the yaml tag is not correctly used. So I am just going to move forward without the underscore

olblak and others added 5 commits January 25, 2023 16:24
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Jan 26, 2023

Once the tests are passing here, I'll merge this pullrequest so I can finalize the npm autodiscovery one

@olblak
Copy link
Member Author

olblak commented Jan 26, 2023

Same remark than here until we release 0.44.0 after FOSDEM. I am open to change the name. Once release it will have to be deprecated

@olblak olblak merged commit aeb247d into updatecli:main Jan 26, 2023
@olblak olblak modified the milestones: 0.45.0, 0.44.0 Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request resource-shell Resource of kind Shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants