Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat/andrews actions #67
base: master
Are you sure you want to change the base?
Feat/andrews actions #67
Changes from 7 commits
dd4af40
c3fc59a
d27f23e
231ffbc
2c44c6b
955708f
3217f61
5e2e781
bffe4ab
a7fbc40
fc1f7d5
307a950
a2ecc8f
2219fdd
b2a032d
d1e7cdb
58b6219
dd1f4a6
0763bf1
9b6abaa
6843b21
a057622
679681a
fd8b982
f27b4a1
ffa9141
7d2808d
39a2bbc
15d2084
adaf88d
a686e09
f04c0b3
d214063
54409b0
22b9ecd
723049f
c9936a8
1a50998
6bd29a3
2ab8bd3
f649fdd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to install the GH CLI at runtime? github-hosted runners should have it installed already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah a github hosted runner would be idea. The repo being downloaded is this terraform-pipeline-executor project written in go. What it's doing under the hood requires the authentications in our self hosted runner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, so this is intended to be ran on self-hosted runners. A pattern I've seen for these types of actions is to check if the tool is already present, and only install if it's not present. If this action is only for use on self-hosted runners then maybe that's overkill, but it would be helpful to make it very obvious that the action shouldn't be used on gh-hosted runners. (maybe that's true of this entire repo and I just missed it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah they're pretty much all meant to be used in a situation where you have to use a self-hosted a runner. The discover-changed action is the only one that's used in a gh hosted runner. I could check for prior existence, however to be prudent, I would also need to check if the version of an already-present binary matches, as the user might want to override the version already installed. That said, for our specific case, if I had it my way, I would have control of the runner image and make sure the gh cli was on there to begin with. I think I'm landing on explaining the intent of use in the README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could also add a
skip-gh-if-already-present
boolean option, whereby if it's present, and the user does not care about the version, then to skip the download of gh.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up doing away with gc cli altogether, and figured out the correct way to do a curl request on private repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm lacking context on what this action is for, but this pattern (
-p
) seems very focused, while the action name implies it's fairly generic. Should the entire pattern be an input parameter?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -p is "Download only assets that match a glob pattern" from this documentation https://cli.github.com/manual/gh_help_reference (search for "gh release download" on that page). This action is very specific in the sense that it downloads a archive of a public or private repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, I missed that this was for downloading a repo archive. I assumed it was for downloading other artifacts from a release. Should the action name indicate that it's for that, specifically, then? Is there a reason to download the release archive of a repo instead of using git operations to pull a repo at a specific tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repo being pulled is a private repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are the path to and name of
terragrunt
parameterized? is it unsafe to assume terragrunt will be configured onPATH
before the action is called?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I parameterized them to avoid assumptions that could not be overwritten. The default value for the terragrunt location is /usr/local/bin (and default name of binary being terragrunt). When used in conjuction with the terragrunt-setup action, the binary will be in the expected location so as not to require overwriting the defaults. However if terragrunt was downloaded or installed by another means, then the terragrunt location can be explicitly specified. Looking at it again, it does seem overly complicated to parameterize both values, and I could consolidate them into a single input, for example "terragrunt-bin-path".