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

runner: support shell parameter #28

Merged
merged 1 commit into from
Apr 30, 2018
Merged

runner: support shell parameter #28

merged 1 commit into from
Apr 30, 2018

Conversation

octplane
Copy link
Contributor

Summary

This PR allows the user to choose a shell to run the task.

shell support single or multiple commands to run the shell:

Run using bash:

  - use: "falcon"
    shell: bash -c
    run:
      - dep status

This PR implements the following feature:

Fixes #27

Checklist

  • Have you followed the guidelines in our CONTRIBUTING guide?
  • Have you fmt your code locally prior to submission (orbit run fmt)?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally (orbit run ci)?
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code

@octplane
Copy link
Contributor Author

Doc is missing though. If you are happy with the changes, I can then add the doc.

@codecov
Copy link

codecov bot commented Apr 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@4c192f9). Click here to learn what that means.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master     #28   +/-   ##
========================================
  Coverage          ?   93.3%           
========================================
  Files             ?       7           
  Lines             ?     224           
  Branches          ?       0           
========================================
  Hits              ?     209           
  Misses            ?       7           
  Partials          ?       8
Impacted Files Coverage Δ
app/runner/runner.go 94.91% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c192f9...87d4c01. Read the comment docs.

@gulien
Copy link
Owner

gulien commented Apr 27, 2018

Interesting option 😃

However, I think running Orbit as mentioned in my comment on issue #27 should be the actual way of doing that.

Still, an interesting option...

Copy link
Owner

@gulien gulien left a comment

Choose a reason for hiding this comment

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

Some minor changes and I think we're good :)

@@ -46,6 +47,10 @@ type (
// printing the available tasks.
Private bool `yaml:"private,omitempty"`

// Shell allows to choose which binary will
// be called to run the commands
Copy link
Owner

Choose a reason for hiding this comment

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

Point required at the end of the sentence #grammarNazi

t.Error("Custom shell task should have failed!")
}

// case 5: uses custom shell with existent shell.
Copy link
Owner

Choose a reason for hiding this comment

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

case 6 here :)

if task.Shell != "" {
shellAndParams := strings.Fields(task.Shell)
shell := shellAndParams[0]
parameters := append(shellAndParams[1:], cmd)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should check the numbers of field (if == 1, this won't work)

Copy link
Contributor Author

@octplane octplane Apr 30, 2018

Choose a reason for hiding this comment

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

Not sure about that: ariane test works correctly, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Indeed... missed it!

- You can now pass a shell parameter to your run task
- support multiple words commands, such as `bash -c`
- added failing an successful tests
@gulien gulien merged commit aa00579 into gulien:master Apr 30, 2018
@octplane
Copy link
Contributor Author

🎉 !

gulien added a commit that referenced this pull request Apr 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants