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

Adds sh action #639

Merged
merged 3 commits into from
May 11, 2019
Merged

Adds sh action #639

merged 3 commits into from
May 11, 2019

Conversation

JayjeetAtGithub
Copy link
Collaborator

Addresses #619

@pep8speaks
Copy link

pep8speaks commented May 6, 2019

Hello @JayjeetAtGithub! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-11 07:14:22 UTC

@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron You may have a look.

Copy link
Collaborator

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

thanks a lot @JayjeetAtGithub! please take a look at the comments

cli/popper/gha.py Outdated Show resolved Hide resolved
@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/issue619 branch 3 times, most recently from 9ef0343 to e636038 Compare May 8, 2019 20:04
@JayjeetAtGithub
Copy link
Collaborator Author

JayjeetAtGithub commented May 9, 2019

@ivotron Please review ! Thanks.

Copy link
Collaborator

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

thanks a lot @JayjeetAtGithub ! Please take a look at the comments. In addition, would it be possible to update the docs, in particular the end of this section (where we have a NOTE). What we need to specify is that for actions running on the host, the $PWD is set to different values depending on whether an action is defined (i.e. an action folder is created), or uses="sh" is used. In the former, $PWD is set to the folder of the action, whereas in the latter it is set to the root of the project

cli/popper/gha.py Outdated Show resolved Hide resolved

if not self.dry_run:
os.chdir(cwd)
if cmd[0].startswith('./'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the case of uses = "sh", this does not apply, since we are running from the root of the project

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we need to execute a bash script located in the root, then how to reference it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

do you mean something like the following?

action "run on host" {
  uses = "sh"
  runs = "./scriptatroot"
}

Copy link
Collaborator

@ivotron ivotron May 9, 2019

Choose a reason for hiding this comment

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

can also be:

action "run on host" {
  uses = "sh"
  runs = "scriptatroot"
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ivotron Thanks! Got it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, that's a good idea!

@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron Please check ! Thanks

@ivotron
Copy link
Collaborator

ivotron commented May 11, 2019

thanks @JayjeetAtGithub . Do you think you can add the changes to the docs in this PR as well? What I mentioned above.

@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron Updated the docs.

@ivotron ivotron merged commit 3787577 into getpopper:master May 11, 2019
@ivotron
Copy link
Collaborator

ivotron commented May 11, 2019

thanks a lot @JayjeetAtGithub !

@JayjeetAtGithub JayjeetAtGithub deleted the jayjeet/issue619 branch August 18, 2019 20:10
ivotron pushed a commit that referenced this pull request May 24, 2020
ivotron pushed a commit that referenced this pull request May 25, 2020
ivotron pushed a commit that referenced this pull request May 25, 2020
ivotron pushed a commit that referenced this pull request May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants