feat(otto): support github app auth and improve secrets management #10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change enhances Otto's configuration system by: 1. Separating sensitive data from regular configuration 2. Adding support for GitHub App authentication via go-githubauth 3. Supporting environment variables for secrets 4. Improving security with proper secrets management Related to issue open-telemetry#6
- Move configuration to dedicated config package - Create secrets package with multiple implementations - Implement 1Password integration for secure secrets storage - Organize code into cleaner, more maintainable structure - Add comprehensive tests for all new components - Add example configuration files
- Check error returns from fmt.Sscanf calls - Use github.Ptr instead of deprecated github.String - Fix unused variables - Add proper error handling for tx.Rollback - Add error handling for write operations - Add .gitignore file 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Set longer timeout to prevent CI timeouts - Include only the specific linters that are causing issues - Exclude errcheck in test files 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR separates sensitive data from regular configuration and improves security by introducing dedicated secrets management, including support for GitHub App authentication via go‐githubauth and flexible configuration via environment variables, secrets file, or 1Password integration.
- Removed otel resource assignment for deprecated ottoResource
- Modified server instantiation to use a secrets manager for retrieving the webhook secret
- Added tests and implementations for 1Password and file-based secrets management, along with corresponding configuration updates
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| otto/internal/telemetry.go | Removed obsolete ottoResource assignment |
| otto/internal/server.go | Updated server constructor to use secrets manager for webhook secret |
| otto/internal/secrets/* | Added and enhanced secrets management implementations and tests |
| otto/internal/app.go | Updated application initialization to support GitHub App authentication |
| otto/internal/config/*.go | Adjusted configuration loading with environment variable and file fallbacks |
| otto/cmd/otto/main.go | Modified main to load both config and secrets files |
| otto/README.md | Updated documentation for configuration and secrets management |
Files not reviewed (2)
- otto/.gitignore: Language not supported
- otto/go.mod: Language not supported
- Address PR review comment on span context usage - Explain why we're not using the returned context currently - Maintain linter compatibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
Thanks for the review comment! I've added an explicit comment explaining why we're not currently using the returned context from OttoTracer().Start(). Since the context is not used further in this method, capturing it leads to ineffectual assignment linter errors. I've added comments to explain that this is intentional and to maintain consistency with other tracing patterns. |
austinlparker
changed the title
jaronoff97
reviewed
Apr 29, 2025
jaronoff97
reviewed
Apr 29, 2025
jaronoff97
left a comment
jaronoff97
left a comment
There was a problem hiding this comment.
a few collected thoughts, i think the main thing is around globals and some simplification for setting config from environment. Overall though, nothing massively blocking. I think it would be good to add in a linter and ensure formatting.
- Fix duplicate imports in telemetry.go - Add ReadHeaderTimeout to server.go to prevent Slowloris attacks - Replace fmt.Sscanf with strconv.ParseInt - Fix error handling with errors.Is - Add proper error checking for writes - Add package comments - Improve golangci-lint configuration - Replace os.Setenv with t.Setenv in tests - Document exported methods 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace all global variables with proper dependency injection - Create encapsulating structs for Database, TelemetryManager, and ModuleRegistry - Cache environment variables during initialization in secret managers - Add Kubernetes-compatible health check endpoints - Remove legacy health endpoint - Fix formatting issues with gofumpt and golines - Use standard library constants for HTTP methods 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR enhances secrets management by separating sensitive data from regular configuration and adds support for GitHub App authentication. Key changes include a refactor of module registration via a dedicated ModuleRegistry, reorganized database and configuration handling, and updates to initialize the GitHub client using new secrets-based credentials.
Reviewed Changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| otto/internal/modules_test.go | Updated test setup to use the app’s module registry. |
| otto/internal/modules.go | Refactored global module registry into a ModuleRegistry structure. |
| otto/internal/db.go | Replaced global DB access with a Database type for connection handling. |
| otto/internal/config/config_test.go | Added tests for config loading and default application settings. |
| otto/internal/config/config.go | Updated configuration loading with defaults and logging summary. |
| otto/internal/config.go | Removed legacy configuration code. |
| otto/internal/commands.go | Enhanced LogSlashCommand to optionally use telemetry for spans. |
| otto/internal/app.go | Integrated secrets management, GitHub App auth, and module registry. |
| otto/config.example.yaml | Provided updated non-secret configuration examples. |
| otto/cmd/otto/main.go | Updated configuration loading and explicit module registration. |
| otto/README.md | Expanded documentation on configuration, secrets, and deployment. |
| otto/CLAUDE.md | Revised build and style guidelines for development. |
| otto/.golangci.yml | Upgraded linter configuration and tool versions. |
| .github/workflows/test.yml | Updated workflow to use the latest golangci-lint-action version. |
Files not reviewed (2)
- otto/.gitignore: Language not supported
- otto/go.mod: Language not supported
- Add improved NewApp function documentation with parameter descriptions - Enhance GitHub App credential validation to check for partially provided credentials - Add detailed error message when credentials are incomplete 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Fixes #6