-
Notifications
You must be signed in to change notification settings - Fork 5
feat: support additional GitHub Action parameters #70
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
Conversation
…b_repo, patcher_version, terrapatch_github_repo, terrapatch_version)
ZachGoldberg
left a comment
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.
If we're going to add this customization, we need to support more than just cloud GitHub orgs. We should be able to also support enterprise GitHub (e.g. git.internal.acme.com), GitLab or potentially non-SCM hosted.
|
Hey folks, what are next steps here? |
- Add new action inputs for SCM configuration (scm_base_url, scm_type, scm_api_version) - Abstract SCM provider logic with GitHub and GitLab implementations - Update binary download logic to work with different SCM providers - Maintain backward compatibility with existing GitHub.com usage - Add comprehensive documentation with enterprise usage examples Co-Authored-By: Josh Padnick <[email protected]>
- Add getDefaultApiVersion() function to automatically detect API versions - Remove need for users to manually specify scm_api_version in most cases - Update documentation to reflect auto-detection behavior - Simplify usage examples by removing scm_api_version parameter Co-Authored-By: Josh Padnick <[email protected]>
- github_token → auth_token - github_org → scm_org - patcher_github_repo → patcher_git_repo - terrapatch_github_repo → terrapatch_git_repo - pull_request_branch → pr_target_branch - pull_request_title → pr_title - Remove read_token and update_token inputs - Reorder scm_ variables for better organization - Update all documentation and examples Co-Authored-By: Josh Padnick <[email protected]>
- Updated package.json version from 2.13.0 to 3.0.0 - Rebuilt dist/index.js to include new version - Breaking changes: renamed input parameters for multi-SCM support Co-Authored-By: Josh Padnick <[email protected]>
|
@ZachGoldberg I worked with Devin.ai to generate this, reviewed the code, and it looks pretty good to me. The one thing I haven't done is test this manually. Any recommendations on how best to do that? Note that this would represent a breaking change given the interface change. |
- Modified GitHubProvider.validateAccess to allow gruntwork-io repos with limited tokens - Improved error handling in getReleaseByTag with specific error messages - This fixes the CI failure where CI_READONLY_READ_TOKEN lacks gruntwork-io access Co-Authored-By: Josh Padnick <[email protected]>
Co-Authored-By: Josh Padnick <[email protected]>
…15.2 - Fix GitHubProvider constructor to use https://api.github.com for github.com instead of undefined - Update patcher version from v0.15.1 to v0.15.2 across all files - This should resolve the CI 404 authentication errors Co-Authored-By: Josh Padnick <[email protected]>
Co-Authored-By: Josh Padnick <[email protected]>
Co-Authored-By: Josh Padnick <[email protected]>
dist/index.js
Outdated
| const PATCHER_VERSION = core.getInput("patcher_version") || "v0.15.1"; | ||
| const TERRAPATCH_GITHUB_REPO = "terrapatch-cli"; | ||
| const TERRAPATCH_VERSION = "v0.1.6"; | ||
| const GRUNTWORK_GITHUB_ORG = core.getInput("scm_org") || "gruntwork-io"; |
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.
This should be PATCHER_ORG (or PATCHER_GROUP for gitlab... probably fine to just call it org)
dist/index.js
Outdated
| const TERRAPATCH_GITHUB_REPO = "terrapatch-cli"; | ||
| const TERRAPATCH_VERSION = "v0.1.6"; | ||
| const GRUNTWORK_GITHUB_ORG = core.getInput("scm_org") || "gruntwork-io"; | ||
| const PATCHER_GITHUB_REPO = core.getInput("patcher_git_repo") || "patcher-cli"; |
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.
Should be PATCHER_GIT_REPO since we're now also supporting gitlab
src/action.ts
Outdated
| const GRUNTWORK_GITHUB_ORG = core.getInput("scm_org") || "gruntwork-io"; | ||
| const PATCHER_GITHUB_REPO = core.getInput("patcher_git_repo") || "patcher-cli"; | ||
| const PATCHER_VERSION = core.getInput("patcher_version") || "v0.15.2"; | ||
| const TERRAPATCH_GITHUB_REPO = core.getInput("terrapatch_git_repo") || "terrapatch-cli"; |
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.
If we're supporting gitlab for terrapatch then it should also support an ORG and a SCM URL
src/action.ts
Outdated
| }); | ||
| } | ||
|
|
||
| async getReleaseByTag(owner: string, repo: string, tag: string): Promise<any> { |
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.
Promise<any> is a very loose interface, we should define an actual return type here so calling code down below is guaranteed correct
src/action.ts
Outdated
|
|
||
| const re = new RegExp(`${osPlatform()}.*${arch()}`); | ||
| const asset = getReleaseResponse.data.assets.find((obj: any) => re.test(obj.name)); | ||
| const asset = release.assets.find((obj: any) => re.test(obj.name)); |
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.
This is where we need that type on the return of getReleaseByTag -- there's no guarantee github and gitlab both have a name property from their apis
…ownloads - Change downloadScmBinary to use asset.url (API endpoint) instead of asset.browser_download_url - GitHub API docs show browser_download_url is for browser downloads, asset.url is for programmatic downloads - This should resolve the 404 error when downloading from private repositories like gruntwork-io/patcher-cli - Maintains existing Bearer token authentication and Accept: application/octet-stream header Co-Authored-By: Josh Padnick <[email protected]>
…support - Fix broken variable references (GRUNTWORK_GITHUB_ORG → PATCHER_ORG, etc.) - Replace Promise<any> with proper TypeScript interfaces (Release, ReleaseAsset) - Add SCM support for terrapatch with dedicated org input - Maintain working authentication fix for private repository downloads Addresses feedback from ZachGoldberg in PR comments #5-#10 Co-Authored-By: Josh Padnick <[email protected]>
- Add missing terrapatch_scm_org input parameter to documentation - Ensures all action inputs are properly documented in README - Description matches action.yml specification Co-Authored-By: Josh Padnick <[email protected]>
|
You know, it just hit me: Why would a GitLab user be accessing a GitHub Action in the first place? If you're on GitHub.com, GitHub Enterprise, or GitHub Enterprise self-hosted, you'd use this GitHub Action. But if you're on Gitlab, you'd need some kind of hacky workaround for supporting a GitHub Action. Should we update this PR to support GitHub only and remove all support for GitLab? |
|
FYI, this was tested for GitHub.com at https://github.com/gruntwork-io-demo/demo2-infrastructure-live-root/actions/runs/17332775908. |
- Remove hardcoded 'gruntwork-io' check in GitHubProvider.validateAccess - Custom organizations now get warning instead of error on 404 - Maintains backward compatibility for gruntwork-io repositories - Fixes GitHub Enterprise authentication issue reported by user Co-Authored-By: Josh Padnick <[email protected]>
…on issues - Update GitHubProvider.validateAccess error message to suggest checking token permissions first - Provide specific guidance about potential causes (repo doesn't exist, token lacks access, missing 'repo' scope) - Direct users to verify token permissions before contacting Gruntwork support - Addresses GitHub Enterprise instances with restrictive token permissions Co-Authored-By: Josh Padnick <[email protected]>
…y tools - Add isGruntworkTool() helper to categorize tools by organization - Update downloadAndSetupTooling() to accept both user SCM and GitHub.com providers - Create separate GitHub.com provider for minamijoyo/tfupdate and minamijoyo/hcledit - Gruntwork tools (patcher-cli, terrapatch-cli) use GitHub Enterprise - Third-party tools use public GitHub.com regardless of user's SCM configuration Co-Authored-By: Josh Padnick <[email protected]>
- Add validate-github-access.yml workflow for testing GitHub tokens - Support both GitHub.com and GitHub Enterprise instances - Parameterized inputs for SCM base URL, organization, repository, and token - Comprehensive testing of repository access, release access, and token permissions - Clear, actionable error messages with troubleshooting guidance - Helps users validate their tokens before using patcher-action Co-Authored-By: Josh Padnick <[email protected]>
…nsistency - Add TFUPDATE_ORG constant to match HCLEDIT_ORG pattern - Reorder constants to follow PATCHER_*, TERRAPATCH_*, TFUPDATE_*, HCLEDIT_* pattern - Update downloadAndSetupTooling to use TFUPDATE_ORG for tfupdate tool - Improves code organization and maintainability Co-Authored-By: Josh Padnick <[email protected]>
…; no behavior change Co-Authored-By: Josh Padnick <[email protected]>
…public 403/404; clearer errors and preflight; prefer browser_download_url Co-Authored-By: Josh Padnick <[email protected]>
…ownload Url or asset Url.
1. The program couldn't switch between downloading via HTTP and HTTPS. 2. If downloading from a public github repo with a token fails, retry without the repo.
yhakbar
left a comment
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 think my comments are mostly nits.
| const headers: Record<string, string> = {}; | ||
|
|
||
| // Asset API URL: always needs proper authentication | ||
| if (token) { |
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 confused. Why are we trying to not set the header if token isn't specified? Shouldn't it always be specified?
| const status = (err?.status || err?.code || "").toString(); | ||
| const isAuthIssue = status === "404" || status === "403"; | ||
|
|
||
| if (isPublicTool && authHeader && isAuthIssue) { |
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.
NIT: This block is kind of hard to read. Could we replace the else's with early returns/throws?
| tag: string | ||
| ): Promise<DownloadedBinary> { | ||
| if (path.extname(asset.name) === ".gz") { | ||
| await exec.exec(`mkdir /tmp/${binaryName}`); |
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.
NIT: Why isn't this done with Node?... I know it isn't part of your PR, but it's strange.
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, good point. Won't fix for now, though due to appetite limits.
| ): Promise<DownloadedBinary> { | ||
| if (path.extname(asset.name) === ".gz") { | ||
| await exec.exec(`mkdir /tmp/${binaryName}`); | ||
| await exec.exec(`tar -C /tmp/${binaryName} -xzvf ${downloadedPath}`); |
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.
NIT: Why isn't this done with Node?... I know it isn't part of your PR, but it's strange.
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, good point. Won't fix for now, though due to appetite limits.
| }; | ||
|
|
||
| const userGitHubProvider = createGitHubProvider(githubConfig); | ||
| core.debug(`Configured github_base_url: ${githubBaseUrl}`); |
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.
What are these debugs for?
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.
GitHub Actions has a very frustrating feedback loop, so I was...liberal with the debug statements. I figure they're useful for customer assistnace, too.
Co-authored-by: Yousif Akbar <[email protected]>
|
@yhakbar @wakeful I tried my best but I couldn't quite get this PR across the finish line, and I can't spare the additional time needed to get it done. In this comment, I'll outline what remains to be done. The main objective here was to allow GitHub Enterprise self-hosted (GHES) users to use Patcher. This PR succeeds in making a backwards-compatible change that has been verified to work for GitHub.com users, however there is still an open issue with GitHub Enterprise support. What's needed to support GitHub EnterpriseFirst, I recommend reading "Step-by-step setup for GitHub Enterprise users" in https://github.com/gruntwork-io/docs/pull/2723/files to understand what's needed to get GitHub Enterprise set up. In order to meet these requirements, I used http://ghe.internal.gruntwork.io/, where I made the following changes:
Challenges with GitHub EnterpriseTo support GitHub Enterprise, the user needs to be able to:
Ultimately, I ran out of time on (3) and (4). The main challenge and the key insight here is that a GHES access token is not recognized by GitHub.com. This means that users need to create a GHES token to achieve (1), but importantly need to not use that token to achieve (2). Otherwise, GitHub will attempt to authenticate using a token that's not recognized. I managed to solve (2) by telling Octokit to hit the GitHub API differently depending on whether you're download a Gruntwork tool from (1) or a public tool from (2). But when it came to (3), the issue was that Patcher has its own logic about how it recognizes the GitHub token. By default, when Patcher attempts to fetch Terragrunt, it will use the GitHub Enterprise token, and that token will fail to be recognized. This (3) and (4) are the only remaining parts of this to solve. Validating the solutionTesting a GitHub Action is already tricky and we don't have a great test harness in place already. Testing a GitHub Action within GitHub Enterprise is also somewhat painful, so I ultimately developed a manual validation procedure that works as follows: Validating the GitHub Actions workflow for GitHub.com
Validating the GitHub Actions workflow for GitHub EnterpriseThis was much trickier. Here's the order of how I was validating things.
The current blocker is that this fails at step 3 under "Challenges with GitHub Enterprise." Note that you can find access credentials to http://ghe.internal.gruntwork.io/ in 1Password. DocsI captured most of what I learned in gruntwork-io/docs#2723. This was initially generated by AI, but then I manually edited it. This should not need much, if any, changes, beyond reflecting any fixes needed here. Other notes
Next stepsThe whole point of this undertaking was to support GitHub Enterprise, and alas we are still not there. As such, I humbly request that @wakeful run with things from here. This is something we committed to a customer 2.5 weeks ago and have still not delivered, so this is timely. I recommend looking into how Patcher is using the GitHub auth token, and how it should behave in GitHub Enterprise. Ideally, we can avoid updating Patcher to be aware of GitHub Enterprise and can instead update this patcher-action to support downloading with the right auth profile. Once this PR is merged, DEV-1042 is done, and we can then update gruntwork-io/docs#2723 as needed, and merge that. At that point, DEV-1044 is done, and our work will be completed. Thanks for all your efforts, and please let me know if you need any input or someone to bounce ideas off of. I'm very sorry that I couldn't get this across the finish line, and thanks again for your help. |
yhakbar
left a comment
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.
LGTM
yhakbar
left a comment
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 spoke too soon. Those CI errors look real:
https://github.com/gruntwork-io/patcher-action/actions/runs/17451017820/job/49555525704?pr=70
yhakbar
left a comment
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 going to assume this has been tested, and works.
|
@yhakbar The key question is if this GitHub Action, as it is, can be cloned to a GitHub Enterprise instance, and if it can then be referenced from a GitHub Enterprise repo and successfully run Patcher. So far, I don't see any change in behavior in how Patcher is downloaded, unless the consuming GitHub Action or env vars are somehow fixing the issue. |
Refactor: Rename inputs to be SCM-agnostic for multi-SCM support
Summary
This PR renames all GitHub-specific input parameters to be generic and intuitive for multi-SCM support, following Josh's feedback to make the action more user-friendly. The changes include:
Input parameter renaming:
github_token→auth_tokengithub_org→scm_orgpatcher_github_repo→patcher_git_repoterrapatch_github_repo→terrapatch_git_repopull_request_branch→pr_target_branchpull_request_title→pr_titleSimplified authentication: Removed
read_tokenandupdate_tokeninputs - now uses singleauth_tokenfor all operationsImproved organization: Reordered scm_ variables as:
scm_type,scm_base_url,scm_org,scm_api_versionUpdated all references: Modified
src/action.ts,README.md, workflow files, and examples to use new input namesReview & Testing Checklist for Human
This is a high-risk change due to its breaking nature and extensive scope. Please verify:
Notes
.github/workflowsandexamples/have been updated to use new input namesauth_tokeninput is now required (was previously optional with fallback to${{ github.token }})Link to Devin run: https://app.devin.ai/sessions/684d0b5b49164cf3b5aaa4b9f74ed7da
Requested by: Josh Padnick (@josh-padnick)