Feat/andrews actions #67
Conversation
github-download-release/action.yml
Outdated
| runs: | ||
| using: composite | ||
| steps: | ||
| - name: Download github cli |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
I ended up doing away with gc cli altogether, and figured out the correct way to do a curl request on private repo.
github-download-release/action.yml
Outdated
| run: | | ||
| export RELEASE_TAG=$(echo "${RELEASE_NAME}" | sed s/^v//g) | ||
| printf "Downloading release '%s' from repo '%s' ...\n" "${RELEASE_NAME}" "${GITHUB_REPO_OWNER}/${GITHUB_REPO}" | ||
| ./gh release download --repo "${GITHUB_REPO_OWNER}/${GITHUB_REPO}" "${RELEASE_NAME}" -p "${GITHUB_REPO}_${RELEASE_TAG}_${DOWNLOAD_FORMAT_EXT}" |
There was a problem hiding this comment.
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.
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.
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.
The repo being pulled is a private repo
terragrunt-run-all/action.yml
Outdated
| iam-role: | ||
| description: The iam role to assume | ||
| required: true | ||
| bin-name: |
There was a problem hiding this comment.
why are the path to and name of terragrunt parameterized? is it unsafe to assume terragrunt will be configured on PATH before the action is called?
There was a problem hiding this comment.
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".
terragrunt-run-all/action.yml
Outdated
| env: | ||
| TERRAGRUNT_IAM_ROLE: ${{ inputs.iam-role }} | ||
| run: | | ||
| ${{ inputs.terragrunt-bin-path }} run-all ${{ inputs.run-all-action }} --terragrunt-non-interactive --terragrunt-working-dir ${{ inputs.folder }} ${{ inputs.additional-options-args }} |
There was a problem hiding this comment.
If this action is wrapping a single run step, does it still make sense to have it as a separate action? Is it likely that this action will do other things before the run-all?
There was a problem hiding this comment.
Do you meant there would be some additional parameters between ${{ inputs.terragrunt-bin-path }} and "run-all"? Or that there might be an additional step before "terragrunt run-all"? If it's the latter, my thinking with this action is only used to run the terragrunt plan command, and abstract away the nuances for doing that
There was a problem hiding this comment.
The more I think about it, terragrunt run-all commands can get too nuanced to handle with an overly simple action such as this, so I think I'm going to remove it from this pr.
|
@steve-liatrio I made some updates. I haven't finished the README files. How do you feel about merging this into master? |
|
Hey @andrewhaller, sorry I forgot to keep track of this PR. If there's somebody who's more familiar with this change it might be a good idea to get them to review it, as I still don't feel like I have a lot of context for the change. I would recommend READMEs, though. It would help for those without context 🙂 |
No description provided.