feat: GitHub App Support #16
Conversation
WalkthroughThis update corrects a spelling error in deployment notifications and introduces several formatting improvements across documentation and Terraform files. New configuration options for GitHub App authentication and GitHub notifications have been added. The changes include conditional logic to inject GitHub notification credentials from AWS SSM, updates to provider configuration for GitHub App support, and modifications to ingress annotations. Minor reformatting in comments and indentation is also applied. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terraform
participant AWS_SSM
participant Notifications_Module
User->>Terraform: Set var.github_notifications_app_enabled = true
Terraform->>AWS_SSM: Retrieve github_notifications_app_private_key
AWS_SSM-->>Terraform: Return private key value
Terraform->>Notifications_Module: Inject GitHub notifications credentials into notification templates and notifiers
sequenceDiagram
participant User
participant Terraform
participant AWS_SSM
participant GitHub_Provider
User->>Terraform: Set var.github_app_enabled = true
Terraform->>AWS_SSM: Retrieve ssm_github_app_private_key (if required)
AWS_SSM-->>Terraform: Return GitHub App private key
Terraform->>GitHub_Provider: Configure dynamic app_auth block for GitHub App authentication
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: 1
🧹 Nitpick comments (8)
src/CHANGELOG.md (2)
1-6: Empty Markdown Links in Changelog
There are empty markdown link references (e.g.[]()on lines 1 and 5). Consider providing valid URLs or removing these links if they aren’t needed.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: No empty links
null(MD042, no-empty-links)
5-5: No empty links
null(MD042, no-empty-links)
19-23: Upgrade Actions Update
The upgrade actions now instruct to addgithub_default_notifications_enabled: trueandgithub_webhook_enabled: truewhile removing the previous notifications variables. Please verify that these changes are documented elsewhere (e.g. in an upgrade guide) so users know about the removed variables.🧰 Tools
🪛 LanguageTool
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...1. Update atmos stack yaml config 1. Addgithub_default_notifications_enabled: true2. Addgithub_webhook_enabled: true3. Re...(ENGLISH_WORD_REPEAT_RULE)
src/provider-github.tf (1)
7-25: Introduction of GitHub App Authentication Variables
New variables (github_app_enabled,github_app_id,github_app_installation_id, andssm_github_app_private_key) have been added. Their types, defaults, and descriptions look clear. Please verify that their default values and SSM paths match your deployment expectations for switching from PAT to GitHub App authentication.src/variables-argocd-notifications.tf (1)
1-6: Pre-existing Notification Variables
The existing variable definitions remain clear. This file now also includes new GitHub App–related variables for notifications. Double-check that your documentation reflects these changes so operators understand the new authentication methods for notifications.src/README.md (2)
553-562: Documentation for New GitHub App Variables
New input variables for GitHub App authentication (such asgithub_app_enabled,github_app_id, andgithub_app_installation_idalong with the notifications counterparts) have been added. Ensure that the descriptions here match the implementation insrc/provider-github.tfandsrc/variables-argocd-notifications.tf, and consider adding examples of how to configure them.🧰 Tools
🪛 LanguageTool
[uncategorized] ~557-~557: 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] ~557-~557: 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)
632-633: Image Alt Text Missing
The image on line 632 lacks descriptive alternate text. For better accessibility, consider adding an alt attribute (for example,alt="Cloud Posse Logo").🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
632-632: Images should have alternate text (alt text)
null(MD045, no-alt-text)
src/notifications.tf (2)
41-42: Webhook Nullification When Using GitHub App
For theapp-deploy-succeeded(and similarly for started/failed) template, thewebhookblock is set tonullwhen GitHub App authentication is enabled. This ensures only the proper notification mechanism is active. Confirm that this pattern is applied consistently across all templates.
278-298: Notification Triggers Configuration
The overall configuration for notification triggers (for deployment start, success, and failure) now incorporates conditions based on the application’s state. These detailed trigger conditions seem robust, but please confirm that they capture all state transitions for your use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/CHANGELOG.md(3 hunks)src/README.md(9 hunks)src/context.tf(1 hunks)src/main.tf(1 hunks)src/notifications.tf(8 hunks)src/provider-github.tf(2 hunks)src/resources/argocd-values.yaml.tpl(1 hunks)src/variables-argocd-notifications.tf(1 hunks)src/variables-argocd.tf(1 hunks)src/variables-helm.tf(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
src/CHANGELOG.md
1-1: No empty links
null
(MD042, no-empty-links)
5-5: No empty links
null
(MD042, no-empty-links)
src/README.md
632-632: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
🪛 LanguageTool
src/CHANGELOG.md
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...1. Update atmos stack yaml config 1. Add github_default_notifications_enabled: true 2. Add github_webhook_enabled: true 3. Re...
(ENGLISH_WORD_REPEAT_RULE)
src/README.md
[grammar] ~413-~413: Did you mean the communication tool “Slack” (= proper noun, capitalized)?
Context: ... integrations for each Application with var.slack_notifications_enabled and `var.slac...
(ON_SKYPE)
[grammar] ~413-~413: Did you mean the communication tool “Slack” (= proper noun, capitalized)?
Context: ...h var.slack_notifications_enabled and var.slack_notifications: ```yaml slack_notifica...
(ON_SKYPE)
[grammar] ~551-~551: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...
(TOO_ADJECTIVE_TO)
[uncategorized] ~557-~557: 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] ~557-~557: 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)
⏰ 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 (14)
src/variables-helm.tf (1)
1-1: LGTM: Comment style updated for consistencyThe comment style has been updated from
//to#, which is the recommended comment style in Terraform files.src/variables-argocd.tf (1)
1-1: LGTM: Comment style updated for consistencyThe comment style has been updated from
//to#, which is the recommended comment style in Terraform files.src/context.tf (1)
267-268: LGTM: Indentation improved for better readabilityThe indentation has been adjusted for consistency and better readability.
src/main.tf (1)
54-61: LGTM: GitHub App private key configuration addedThis change adds the GitHub App private key to the credentials template when GitHub notifications app is enabled. This supports the PR objective of implementing GitHub Apps for authentication.
Note that this change relies on the
github_notifications_app_private_keyparameter which should be defined elsewhere in the codebase.src/resources/argocd-values.yaml.tpl (1)
21-21: LGTM: Updated ingress class configurationThe ingress configuration has been updated to use the newer
ingressClassNamefield instead of the deprecatedkubernetes.io/ingress.classannotation. This follows Kubernetes best practices for ingress configuration.src/CHANGELOG.md (1)
26-55: Changes in Notification Configurations
The diff block removes thenotifications_triggers,notifications_templates, andnotifications_notifierssections in favor of new GitHub App–related variables. Ensure that the upstream modules and theargocd-repocomponent are updated accordingly to avoid any configuration mismatches in your deployment.src/provider-github.tf (3)
45-49: Conditional GitHub Token Logic
The locals block now setsgithub_tokentonullwhen GitHub App authentication is enabled. This logic is correct provided that subsequent provider configurations rely on the absence of a PAT.
51-63: Selective SSM Parameter Retrieval
Data sources forgithub_api_key(for PAT) andgithub_app_private_key(for GitHub App authentication) are conditionally created based onvar.github_app_enabled. This ensures that only the relevant secret is retrieved.
66-79: Dynamic App Authentication Block
The dynamic block forapp_authwithin the GitHub provider is well implemented. It provides the necessary fields (id,installation_id, andpem_file) when GitHub App authentication is enabled. Verify that the values provided (especially from SSM) are correct in your environment.src/variables-argocd-notifications.tf (1)
7-30: New GitHub Notifications App Variables
The newly introduced variables (github_notifications_app_enabled,github_notifications_app_id,github_notifications_app_installation_id,ssm_github_notifications_app_private_key) are defined with clear types, defaults, and descriptions. Ensure that the SSM path/argocd/github_notifications/app_private_keyis correct and that these values are propagated to the notifications modules properly.src/README.md (1)
9-10: Component Naming Convention Update
The header now displays the component aseks/argocd(with proper formatting), which improves clarity about the namespace.src/notifications.tf (3)
15-19: Conditional Data Source for GitHub Notifications App Private Key
A new data source foraws_ssm_parameter.github_notifications_app_private_keyis conditionally created when bothlocal.github_notifications_enabledandvar.github_notifications_app_enabledare true. This design ensures that the GitHub App private key is fetched only when needed.
29-32: Conditional GitHub Configuration in Notification Templates
Within thenotifications_templatesmodule, theapp-deploy-succeededtemplate now includes agithubblock only whenvar.github_notifications_app_enabledis true; otherwise, it sets the field tonull. This conditional approach clearly separates the GitHub App and webhook notification modes.
268-274: Notification Template Merging
The merging logic innotifications_template_app_github_commit_statusandnotifications_template_argocd_repo_github_commit_statuscorrectly extends the base template with GitHub-specific paths. Please verify that the constructed URL paths accurately reference the application's repository and commit information.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/CHANGELOG.md (4)
18-23: Upgrade Actions List Clarity
The upgrade actions clearly instruct users to add the new variables (github_default_notifications_enabledandgithub_webhook_enabled) and remove the deprecated notification variables (notifications_triggers,notifications_templates, andnotifications_notifiers). Ensure that these changes are reflected in your Terraform module configurations and any associated documentation.🧰 Tools
🪛 LanguageTool
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...1. Update atmos stack yaml config 1. Addgithub_default_notifications_enabled: true2. Addgithub_webhook_enabled: true3. Re...(ENGLISH_WORD_REPEAT_RULE)
75-95: Features Section and Notification Identifier Consistency
The features section now enumerates the updated GitHub integration capabilities, including Git webhook configuration and GitHub commit status notifications. However, note that the notification identifieron-deploy-succededstill appears here. To maintain consistency with the corrected spelling noted at the top of the changelog, consider updating this identifier toon-deploy-succeededif it is not intended to differ.
108-126: Terraform Variable Structure Update (Notifications Notifiers)
This diff removes theservice_githubblock and renamesservice_webhooktowebhook. These breaking changes require a careful review of any dependent configurations or modules to ensure they reference the newwebhookkey. If migration assistance or additional documentation is needed, please consider updating those areas.
129-154: Terraform Variable Structure Update (Notifications Templates)
The diff removes thegithubconfiguration block from thenotifications_templatesvariable, leaving only thewebhookconfiguration. Verify that any downstream code or documentation previously relying on thegithubstructure is updated to utilize the new consolidated configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CHANGELOG.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
src/CHANGELOG.md
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...1. Update atmos stack yaml config 1. Add github_default_notifications_enabled: true 2. Add github_webhook_enabled: true 3. Re...
(ENGLISH_WORD_REPEAT_RULE)
⏰ 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 (9)
src/CHANGELOG.md (9)
1-6: Changelog Header and Notification Correction Note
The header properly references PR [#16] and documents that the spelling of "succeded" has been corrected to "succeeded" for the notification. Please verify that this correction is uniformly applied across all related modules and documentation.
25-55: Terraform Configuration Diff Details
This diff block accurately reflects the addition of GitHub App–related variables and the removal of legacy notification variables. It is important to confirm that all references to the removed variables are updated in downstream modules and that the new variable names are correctly consumed.
57-59: Secret Relocation Instruction
The instruction to move secrets from
/argocd/notifications/notifiers/service_webhook_github-commit-status/github-token
to
argocd/notifications/notifiers/common/github-token
is clearly stated. Ensure that all related scripts and documentation referencing these paths are updated accordingly.
60-63: Secret Management Commands Verification
The provided bash commands usingchamber read,chamber write, andchamber deleteappear correct. Please double-check that they are executed in the proper sequence to maintain secret continuity during the migration.
65-67: GitHub PAT Creation Instruction
The instruction to create a GitHub PAT with the scopeadmin:repo_hookis straightforward. Confirm that this step is well communicated to teams and note that future authentication may leverage GitHub App credentials.
68-71: SSM PAT Storage Command
Storing the PAT in SSM via the providedchamber writecommand is clearly documented. Ensure that the SSM path/argocd/github/api_keymatches your deployment configuration and any automated scripts using this key.
73-74: Deployment Instruction with Atmos
The note to “Apply changes with atmos” is concise. Please ensure that any prerequisites or contextual instructions for atmos-based deployments are clearly communicated to the end users.
96-102: Bug Fixes Documentation Review
The bug fixes section provides clear information on issues addressed (e.g., pod recreation on config changes, removal of metadata outputs, and legacy helm values). This clarity helps users understand potential impacts when upgrading.
103-107: Breaking Changes Clarity
The breaking changes are well documented, specifically the removal of theservice_githubentry and the renaming ofservice_webhooktowebhookin the notifications variables. Ensure that any downstream modules or automation scripts referencing these variables are updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/CHANGELOG.md (3)
7-10: Clarification on File Renaming Instruction:
The text states, "Thenotifications.tffile has been renamed tonotifications.tf." This is ambiguous—if a renaming was intended, please verify that the correct source and target filenames are provided for clarity.
18-24: Upgrade Actions Formatting:
The upgrade actions list clearly outlines the required modifications (adding new variables and removing legacy ones). However, the nested numbering (e.g., "1. Add ..." within step 1) might cause confusion. Consider revising the numbering format for improved readability.🧰 Tools
🪛 LanguageTool
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...1. Update atmos stack yaml config 1. Addgithub_default_notifications_enabled: true2. Addgithub_webhook_enabled: true3. Re...(ENGLISH_WORD_REPEAT_RULE)
75-92: Notification Feature Consistency:
The features section lists several GitHub notification events. There appears to be an inconsistency with the event nameon-deploy-succededstill present despite the earlier correction. Please verify that all notification event names use the correct spelling ("succeeded") to maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CHANGELOG.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
src/CHANGELOG.md
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...1. Update atmos stack yaml config 1. Add github_default_notifications_enabled: true 2. Add github_webhook_enabled: true 3. Re...
(ENGLISH_WORD_REPEAT_RULE)
⏰ 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 (9)
src/CHANGELOG.md (9)
1-6: Spelling Correction Update:
The changelog now reflects the corrected spelling ("succeeded") for the GitHub deployment notification. Please confirm that all related documentation and configuration references have been updated accordingly.
25-55: Configuration Update Verification:
This diff block introduces the new variablesgithub_default_notifications_enabledandgithub_webhook_enabledwhile removing the legacy notification triggers, templates, and notifiers. Ensure that all downstream dependencies and references have been updated to align with these changes.
57-63: Secret Migration Validation:
The instructions and accompanying bash commands for migrating the GitHub secret are clear. Please verify that all services referencing the old secret path are updated to use the new path (argocd/notifications/notifiers/common/github-token).
65-71: GitHub PAT Creation and Storage:
The steps for creating a GitHub PAT (with theadmin:repo_hookscope) and storing it in SSM are concise and correctly outlined. Ensure that security best practices regarding secret management are being followed.
73-74: Deployment via Atmos:
The instruction to "Apply changes with atmos" is straightforward. Confirm that the deployment process incorporates the updated configurations effectively.
96-102: Bug Fixes Section Clarity:
The bug fixes are clearly and succinctly listed. No changes are needed in this section.
103-107: Breaking Changes Overview:
The breaking changes are effectively communicated—highlighting the removal and renaming of notification variables. Please ensure that all dependent configurations or modules have been updated as a result of these changes.
107-125: Notifier Variable Renaming:
The diff updates thenotifications_notifiersvariable by removing theservice_githubentry and renamingservice_webhooktowebhook. This refinement improves clarity within the configuration. Make sure that all references to these variables in the codebase are adjusted accordingly.
126-153: Template Configuration Update:
Removing thegithubsub-configuration fromnotifications_templatesaligns with the transition from PATs to GitHub Apps. Confirm that downstream systems are updated to reference the newwebhookconfiguration as intended.
|
/terratest |
|
There are no real tests for this component. So we set terratest statuses to successful execution without running any tests |
|
These changes were released in v2.0.0. |
what
why
references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores