Revert "chore: Use GHE fro github enterprise instead of second"#2710
Conversation
This reverts commit 88e8d39.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request renames GitHub Enterprise (GHE) environment variables to a 'SECOND' naming convention (e.g., TEST_GITHUB_SECOND_*) across CI scripts, documentation, and test configurations to support a second controller setup. Feedback indicates several instances where the migration is incomplete: specifically, TEST_GITHUB_SECOND_WEBHOOK_ORG should be added to the required environment variables list, and several test files still reference TEST_EL_WEBHOOK_SECRET instead of the new TEST_GITHUB_SECOND_WEBHOOK_SECRET, which could cause signature verification failures. Additionally, the GHE implementation currently requires a static token, preventing dynamic token acquisition via the GitHub App.
| "TEST_GITHUB_SECOND_WEBHOOK_TOKEN", | ||
| "TEST_GITHUB_SECOND_WEBHOOK_SECRET", |
There was a problem hiding this comment.
The TEST_GITHUB_SECOND_WEBHOOK_ORG environment variable should be included in the requiredEnvs list for the GHE + Webhook case. It is used to populate config.repoOwner (lines 96-100), and if it's missing (along with the fallback), it will lead to an empty organization name, causing test failures or potential panics during string splitting.
"TEST_GITHUB_SECOND_WEBHOOK_TOKEN",
"TEST_GITHUB_SECOND_WEBHOOK_SECRET",
"TEST_GITHUB_SECOND_WEBHOOK_ORG",| "TEST_GITHUB_GHE_WEBHOOK_SECRET", | ||
| "TEST_GITHUB_SECOND_API_URL", | ||
| "TEST_GITHUB_SECOND_EL_URL", | ||
| "TEST_GITHUB_SECOND_TOKEN", |
There was a problem hiding this comment.
Requiring TEST_GITHUB_SECOND_TOKEN here prevents GHE tests from dynamically obtaining a token via the GitHub App, unlike the public GitHub implementation (lines 134-147). This is likely due to setupGithubAppToken (line 156) being hardcoded to use TEST_GITHUB_REPO_INSTALLATION_ID. To support dynamic tokens for GHE, setupGithubAppToken should be updated to use TEST_GITHUB_SECOND_REPO_INSTALLATION_ID when onGHE is true.
| if onGHE && onWebhook { | ||
| repoName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test") | ||
| smeeURL := os.Getenv("TEST_GITHUB_GHE_WEBHOOK_SMEE_URL") | ||
| smeeURL := os.Getenv("TEST_GITHUB_SECOND_WEBHOOK_SMEE_URL") |
There was a problem hiding this comment.
The webhookSecret on line 202 is using TEST_EL_WEBHOOK_SECRET. For GHE tests, this should be updated to use TEST_GITHUB_SECOND_WEBHOOK_SECRET to remain consistent with the second controller's configuration in hack/gh-workflow-ci.sh. Using the common secret will cause signature verification failures in the GHE environment.
| if g.GHE && g.Webhook { | ||
| repoName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test") | ||
| smeeURL := os.Getenv("TEST_GITHUB_GHE_WEBHOOK_SMEE_URL") | ||
| smeeURL := os.Getenv("TEST_GITHUB_SECOND_WEBHOOK_SMEE_URL") |
There was a problem hiding this comment.
The webhookSecret on line 150 is using TEST_EL_WEBHOOK_SECRET. It should be updated to use TEST_GITHUB_SECOND_WEBHOOK_SECRET for GHE tests to match the second controller's secret configuration. Otherwise, webhooks sent to the GHE controller will fail signature validation.
| if g.GHE && g.Webhook { | ||
| repoName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test") | ||
| smeeURL := os.Getenv("TEST_GITHUB_GHE_WEBHOOK_SMEE_URL") | ||
| smeeURL := os.Getenv("TEST_GITHUB_SECOND_WEBHOOK_SMEE_URL") |
There was a problem hiding this comment.
The webhookSecret on line 286 is using TEST_EL_WEBHOOK_SECRET. For GHE tests, this should be changed to TEST_GITHUB_SECOND_WEBHOOK_SECRET to ensure compatibility with the second controller's webhook secret verification.
1 participant
Reverts #2707