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

Fixes #564 Implements popper add #570

Merged
merged 16 commits into from
Apr 9, 2019

Conversation

JayjeetAtGithub
Copy link
Collaborator

@JayjeetAtGithub JayjeetAtGithub commented Mar 27, 2019

Implements the popper add command. On doing popper add cplee/github-actions-demo , the repository is cloned, the main.workflow file searched, scanned, copied to project_root and then the toplevel folder of local actions are also copied which are identified by parsing the workflow.

@pep8speaks
Copy link

pep8speaks commented Mar 27, 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-04-09 06:09:22 UTC

@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron Please have a look !

@JayjeetAtGithub JayjeetAtGithub changed the title Initial version of popper add Fixes #564 Implements popper add Mar 27, 2019
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 @JayjeetAtGithub! The main idea is to use git to clone the repo and copy the workflow file, along with any locally-defined action that the workflow is referring to.

@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron Got it ! Will be updating soon.

@JayjeetAtGithub
Copy link
Collaborator Author

JayjeetAtGithub commented Apr 2, 2019

@ivotron
When we can refer to reuse using ./test in uses.

action "test reuse" {
  uses = "./test"
  runs = "reuse"
}

So, why do we use

action "test reuse" {
  uses = "./ci/test"
  runs = "reuse"
}

This ought to be had asked before, but somehow my eyes couldn't catch it ?
Can you please help me understand this ?

Without having this cleared, i couldnt understand how to copy the actions. Thanks

@ivotron
Copy link
Collaborator

ivotron commented Apr 3, 2019

sure. References in a uses attribute of a .workflow file are relative to the root of the project. So if a workflow file is in /project/path/containing/main.workflow and there is an action in /project/another/path/to/an/action that the workflow needs, it will reference it by doing ./another/path/to/an/action. In the example you wrote, the action is defined in ./ci/test, which is a container-less action executed by the HostRunner class.

The runs attribute of an action block are references to commands within the action. In the example you gave, it is referring to the reuse command (defined in ./ci/test/reuse).

ihth

@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron This is ready for review.

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, this is looking great! Please take a look at the comments

cli/popper/commands/cmd_add.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_add.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_add.py Outdated Show resolved Hide resolved
@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/popper_add branch 2 times, most recently from 2385373 to d7427c7 Compare April 7, 2019 02:52
@JayjeetAtGithub
Copy link
Collaborator Author

JayjeetAtGithub commented Apr 7, 2019

@ivotron I have made the changes. Please have a look.
We can now do popper add github.com/cplee/github-actions-demo@develop,
popper add gitlab.com/cplee/github-actions-demo/.github@develop etc.

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 @JayjeetAtGithub !

cli/popper/commands/cmd_add.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_add.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_add.py Outdated Show resolved Hide resolved
cli/popper/utils.py Show resolved Hide resolved
cli/popper/commands/cmd_add.py Outdated Show resolved Hide resolved
@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron Updated it. Please have a look !

@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron test interrupt is failing sometimes and only for python3.7. Can you please check it once.

@ivotron
Copy link
Collaborator

ivotron commented Apr 9, 2019

@JayjeetAtGithub can you please rebase master? I made some changes to ctrl-c and test interrupt

@JayjeetAtGithub
Copy link
Collaborator Author

JayjeetAtGithub commented Apr 9, 2019

@ivotron rebased..Please check

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 @JayjeetAtGithub! please take a look at the comments

ci/test/add Show resolved Hide resolved
@ivotron ivotron merged commit 864ea81 into getpopper:master Apr 9, 2019
@ivotron
Copy link
Collaborator

ivotron commented Apr 9, 2019

thanks!

@JayjeetAtGithub JayjeetAtGithub deleted the jayjeet/popper_add branch August 18, 2019 20:02
ivotron pushed a commit that referenced this pull request May 24, 2020
Adds 'popper add' subcommand for importing existing workflows to a project.

fixes #564
ivotron pushed a commit that referenced this pull request May 25, 2020
Adds 'popper add' subcommand for importing existing workflows to a project.

fixes #564
ivotron pushed a commit that referenced this pull request May 25, 2020
Adds 'popper add' subcommand for importing existing workflows to a project.

fixes #564
ivotron pushed a commit that referenced this pull request May 25, 2020
Adds 'popper add' subcommand for importing existing workflows to a project.

fixes #564
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