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.

Sign up for GitHub

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

Jump to bottom

feat: Add a new resource of type "shell" #264

Merged
merged 11 commits into from
Aug 30, 2021
Merged

feat: Add a new resource of type "shell" #264

merged 11 commits into from
Aug 30, 2021

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented Aug 19, 2021 •
edited

Add a new resource of type "shell"

Closes #263

What this PR does?

It introduces a new resource of type shell to allow executing a custom command.

  • For sources, this resource uses the stdout of the command to report the value to updatecli, if and only if the exit code is 0, otherwise it fails, and the stderr of the command is reported by updatecli to the end user.

  • For conditions, this resource executes a command with the associated source appended as last argument. If the exit code is 0 then the condition succeed, otherwise it fails.

  • For targets, this resource executes a command with the associated source appended as last argument. Then:

    • If the exit code is zero AND the stdout reports a non empty string, then updatecli reports that there was a change, and the stdout reports what has changed.
    • If the exit code is zero and stdout is empty, then updatecli report that there was no change (but still success)
    • Another exit code means that the target failed.
    • The environment variable DRY_RUN is set either to false or true in the target's command environment

There is an generic example in examples/updateCli.generic/shell/shell.yaml (along with shell scripts).

What this PR does not?

Test

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

go test ./pkg/plugins/shell -v

~95% of the package shell is covered

Additional Information

  • Not tested on Windows, but as it relies on os/exec Golang package AND each command is a single strings, it should work as expected

Potential improvement

  • If someone raises the issue, the command specification could be improved to support "array mode" ala Dockerfile (for entrypoints/CMD in Dockerfiles)

@dduportal
feat: introduces a new resource: "shell"
26d527b 
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal dduportal marked this pull request as ready for review August 23, 2021 14:02
@dduportal dduportal changed the title Add a new resource of type "shell" (WiP) Add a new resource of type "shell" Aug 23, 2021
jlevesy
jlevesy reviewed Aug 23, 2021
View reviewed changes
Copy link
Contributor

@jlevesy jlevesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Some style nits and suggestions, feel free to disregard if not relevant 😄

examples/updateCli.generic/shell/shell.yaml Outdated Show resolved Hide resolved
pkg/core/engine/condition/main.go Outdated Show resolved Hide resolved
pkg/plugins/shell/command.go Outdated Show resolved Hide resolved
pkg/plugins/shell/command.go Outdated Show resolved Hide resolved
pkg/plugins/shell/command.go Outdated Show resolved Hide resolved
pkg/plugins/shell/main_test.go Outdated Show resolved Hide resolved
pkg/plugins/shell/main_test.go Outdated Show resolved Hide resolved
pkg/plugins/shell/main_test.go Outdated Show resolved Hide resolved
pkg/plugins/shell/source.go Outdated Show resolved Hide resolved
pkg/plugins/shell/target.go Outdated Show resolved Hide resolved
@olblak olblak added condition Modify condition resource enhancement New feature or request source target Related to updatecli target labels Aug 24, 2021
olblak and others added 7 commits August 24, 2021 10:16
@olblak @jlevesy
Update pkg/core/engine/condition/main.go
504cbe7 
Co-authored-by: Julien Levesy <jlevesy@gmail.com>
@olblak @jlevesy
Update pkg/plugins/shell/command.go
4c60f6d 
Co-authored-by: Julien Levesy <jlevesy@gmail.com>
@olblak @jlevesy
Update pkg/plugins/shell/command.go
3e40817 
Co-authored-by: Julien Levesy <jlevesy@gmail.com>
@dduportal @jlevesy
Apply suggestions from code review
9bd0e19 
Co-authored-by: Julien Levesy <jlevesy@gmail.com>
@dduportal
Feedbacks from code review
6fd74eb 
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
Feedbacks from code review
bb846d1 
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
Feedbacks from code review
3511568 
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
jlevesy
jlevesy reviewed Aug 24, 2021
View reviewed changes
Copy link
Contributor

@jlevesy jlevesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

pkg/plugins/shell/condition.go Outdated Show resolved Hide resolved
pkg/plugins/shell/main.go Outdated Show resolved Hide resolved
pkg/plugins/shell/main_test.go Outdated Show resolved Hide resolved
dduportal and others added 2 commits August 27, 2021 17:36
@dduportal @jlevesy
Apply suggestions from code review
983d99a 
Co-authored-by: Julien Levesy <jlevesy@gmail.com>
@dduportal
code review feedbacks
303da84 
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor Author

Ping @olblak , it's ready to 🚢 (at least to be reviewed by you :) )

@dduportal dduportal requested a review from olblak August 27, 2021 15:44
jlevesy
jlevesy approved these changes Aug 27, 2021
View reviewed changes
Copy link
Contributor

@jlevesy jlevesy left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Style nit, otherwise LGTM

pkg/core/engine/condition/main.go Outdated Show resolved Hide resolved
@dduportal @jlevesy
Update pkg/core/engine/condition/main.go
562a436 
Co-authored-by: Julien Levesy <jlevesy@gmail.com>
olblak
olblak approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@olblak olblak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice pull request, now I have to upper the quality of mines :D

@dduportal
Copy link
Contributor Author

Nice pull request, now I have to upper the quality of mines :D

Most of the credit goes to @jlevesy ❤️ for his careful reviews, explanation and teaching me :)

@dduportal dduportal merged commit 7761118 into updatecli:main Aug 30, 2021
@dduportal dduportal changed the title Add a new resource of type "shell" feat: Add a new resource of type "shell" Aug 30, 2021
This was referenced Aug 30, 2021
Open
Merged
@dduportal dduportal deleted the feat/resource-shell branch October 19, 2021 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlevesy jlevesy jlevesy approved these changes

@olblak olblak olblak approved these changes

Assignees
No one assigned
Labels
condition Modify condition resource enhancement New feature or request source target Related to updatecli target
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a new resource of type "shell"
3 participants
@dduportal @jlevesy @olblak