feat: Support for GitHub Apps #17
Conversation
WalkthroughThis pull request introduces several Terraform module configuration improvements. A new pattern is added to Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Module as Terraform Module
participant BP as github_branch_protection
Note over User, Module: Set bypass_pull_request_actors variable
User->>Module: Provide pull request bypass actors
Module->>BP: Update required_pull_request_reviews (with pull_request_bypassers)
BP->>GitHub: Apply branch protection rules with bypassers
sequenceDiagram
participant User as User
participant Module as Terraform Module
participant Provider as provider-github.tf
participant SSM as aws_ssm_parameter
User->>Module: Set use_local_github_credentials (true/false)
Module->>Provider: Check use_local_github_credentials flag
alt use_local_github_credentials = true
Provider->>Provider: Set github_token to null
Provider-->SSM: Skip SSM parameter creation
else use_local_github_credentials = false
Provider->>SSM: Retrieve github_api_key value
Provider->>Provider: Set github_token from SSM or override
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Require terratestWonderful, this rule succeeded.This rule require terratest status
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/README.md (2)
144-144: Refined Descriptor Formats Documentation
The updated formatting and description for thedescriptor_formatsinput variable improve clarity. There is no functional change here, just enhanced readability.
141-151: Minor Wording Style Suggestions
A couple of the variable descriptions could be streamlined per static analysis hints. For example, consider replacing phrases like “whether or not” with “whether” for conciseness, and a careful review of punctuation (e.g., adding a comma before “and” when joining two independent clauses) could further polish the documentation.🧰 Tools
🪛 LanguageTool
[style] ~141-~141: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... create_repo | Whether or not to create the repository or use an exis...(WHETHER)
[grammar] ~145-~145: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...(TOO_ADJECTIVE_TO)
[style] ~147-~147: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...D) for.auto-syncdetermines whether or not the ArgoCD application will be automati...(WHETHER)
[style] ~147-~147: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...r/>ignore-differencesdetermines whether or not the ArgoCD application will ignore the ...(WHETHER)
[uncategorized] ~148-~148: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...se. It is optional to provide this value and it can also be sourced from the `GITHUB...(COMMA_COMPOUND_SENTENCE)
[style] ~148-~148: Reusing ‘It’ could be redundant. Try omitting the pronoun.
Context: ...t is optional to provide this value and it can also be sourced from the `GITHUB_BASE_U...(SUBJECT_DROP)
[grammar] ~151-~151: It seems that a pronoun is missing.
Context: ... defined in theeks/argocdcomponent. If want to add additional notifications, i...(IF_VB)
🪛 markdownlint-cli2 (0.17.2)
147-147: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)src/CHANGELOG.md(1 hunks)src/README.md(2 hunks)src/context.tf(1 hunks)src/main.tf(1 hunks)src/provider-github.tf(1 hunks)src/variables.tf(2 hunks)src/versions.tf(0 hunks)
💤 Files with no reviewable changes (1)
- src/versions.tf
🧰 Additional context used
🪛 LanguageTool
src/README.md
[style] ~141-~141: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... create_repo | Whether or not to create the repository or use an exis...
(WHETHER)
[grammar] ~145-~145: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...
(TOO_ADJECTIVE_TO)
[style] ~147-~147: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...D) for.auto-sync determines whether or not the ArgoCD application will be automati...
(WHETHER)
[style] ~147-~147: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...r/>ignore-differences determines whether or not the ArgoCD application will ignore the ...
(WHETHER)
[uncategorized] ~148-~148: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...se. It is optional to provide this value and it can also be sourced from the `GITHUB...
(COMMA_COMPOUND_SENTENCE)
[style] ~148-~148: Reusing ‘It’ could be redundant. Try omitting the pronoun.
Context: ...t is optional to provide this value and it can also be sourced from the `GITHUB_BASE_U...
(SUBJECT_DROP)
[grammar] ~151-~151: It seems that a pronoun is missing.
Context: ... defined in the eks/argocd component. If want to add additional notifications, i...
(IF_VB)
🪛 markdownlint-cli2 (0.17.2)
src/README.md
147-147: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (11)
.gitignore (1)
78-78: Added ignore pattern for account-map.The addition of
**/account-mapensures that any directories or files named "account-map" are ignored by Git, which helps prevent unintentional versioning of local configuration files.src/context.tf (1)
267-268: Improved formatting for descriptor_formats block.Adjusting the indentation for the
formatandlabelskeys improves readability without affecting functionality.src/main.tf (1)
88-88: Enhanced branch protection with bypass capability.The new attribute
pull_request_bypassers = var.bypass_pull_request_actorsin therequired_pull_request_reviewsblock adds flexibility by allowing designated users or teams to bypass pull request reviews. Please verify that the GitHub provider supports this attribute.src/CHANGELOG.md (1)
1-6: Updated changelog with GitHub Apps support and spelling correction.The new section documents the PR for the
argocd-github-repocomponent and corrects the spelling from "succeded" to "succeeded" in the notification. This provides clear context and references the related pull requests.src/variables.tf (3)
183-184: Corrected spelling in GitHub notification annotations.The change fixes the typo in the
github_notificationsvariable, updating "on-deploy-succeded" to "on-deploy-succeeded" for both the app-repo and argocd-repo notifications.
200-204: Added variable for bypass pull request actors.The new variable
bypass_pull_request_actors(a list of strings) allows users to specify GitHub usernames and team slugs that can bypass pull request requirements, enhancing branch protection rules.
205-209: Introduceduse_local_github_credentialsvariable.This boolean variable enables the use of local GitHub credentials from environment variables instead of pulling them from SSM, which supports flexibility in environments where local credential management is preferred.
src/provider-github.tf (2)
2-4: Clear GitHub Token Assignment Logic
The updated logic correctly distinguishes between using local credentials and fetching the token via SSM. Whenvar.use_local_github_credentialsis true, thegithub_tokenis set tonull(which should disable remote token usage), otherwise it falls back to using either the override or the SSM value. Please ensure that downstream consumers oflocal.github_tokencorrectly handle anullvalue.
8-8: Appropriate SSM Parameter Creation
The conditional for thecountattribute now correctly prevents creating the SSM parameter when local credentials are in use (i.e. whenvar.use_local_github_credentialsis true). This change aligns well with the new feature design.src/README.md (2)
139-139: Documenting Bypass Actors
The addition of thebypass_pull_request_actorsvariable is clear and valuable for allowing specific GitHub users or teams to bypass pull request requirements. Its documentation and default value ([]) appear appropriate.
177-177: Introducing Local GitHub Credentials
The new input variableuse_local_github_credentialsis clearly documented as a boolean option to opt for local GitHub credentials instead of using SSM. This addition nicely complements the changes inprovider-github.tf. Ensure that users are aware of the implications (e.g., that providing anulltoken and skipping SSM may affect authentication in certain environments).
|
/terratest |
|
These changes were released in v2.0.0. |
what
use_local_github_credentialssucceedwhy
references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores