feat: add service account support#152
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #152 +/- ##
==========================================
+ Coverage 96.73% 97.23% +0.50%
==========================================
Files 14 15 +1
Lines 673 723 +50
==========================================
+ Hits 651 703 +52
+ Misses 22 20 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The as_json method alone is not sufficient for direct .to_json calls. Ruby's JSON library requires an explicit to_json method to properly serialize custom objects.
Allow passing credentials directly to Tracker.new instead of requiring
them to be nested in the config hash. This provides a cleaner API:
Before:
tracker = Tracker.new(token, nil,
local_flags_config: { credentials: creds },
remote_flags_config: { credentials: creds })
After:
tracker = Tracker.new(token, nil,
credentials: creds,
local_flags_config: {},
remote_flags_config: {})
Credentials in config still take precedence if both are provided,
allowing different credentials per provider if needed.
Credentials don't need to be part of the config hash - they're a fundamental authentication parameter. This simplifies the API: - Credentials are passed directly as a parameter to the flags providers - No more confusing config[:credentials] nesting - Config hash is only for provider-specific settings (api_host, timeouts, etc.) - Cleaner separation of concerns This removes 30+ lines of unnecessary complexity.
Update test setup to pass nil for credentials parameter in the new 5-argument signature.
There was a problem hiding this comment.
Pull request overview
Adds first-class “service account” support across import and feature flags by introducing a credentials object, wiring it through the tracker/providers, and updating docs + tests to demonstrate and validate the new flow.
Changes:
- Introduce
Mixpanel::ServiceAccountCredentialsand serialize it into import messages. - Update import pipeline (
Events#import→Consumer#send!) to send either legacyapi_keyor service account fields. - Thread optional credentials into local/remote flags providers and use them for HTTP Basic Auth, plus README usage examples and new/updated specs.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/mixpanel-ruby/tracker_spec.rb | Adds spec asserting credentials are passed into flags providers. |
| spec/mixpanel-ruby/flags/remote_flags_spec.rb | Updates constructor usage and adds auth test for remote flags with credentials. |
| spec/mixpanel-ruby/flags/local_flags_spec.rb | Updates constructor usage and adds auth test for local flags with credentials. |
| spec/mixpanel-ruby/events_spec.rb | Adds import message-shape test for service account credentials (currently missing a require). |
| spec/mixpanel-ruby/credentials_spec.rb | New unit tests for credentials validation + JSON serialization. |
| spec/mixpanel-ruby/consumer_spec.rb | Adds test ensuring import requests include service account fields. |
| Readme.rdoc | Documents service account usage for import and feature flags. |
| lib/mixpanel-ruby/tracker.rb | Adds credentials: keyword and passes it to flags providers; updates import docs/signature. |
| lib/mixpanel-ruby/flags/remote_flags_provider.rb | Extends initializer to accept credentials and forward into provider config. |
| lib/mixpanel-ruby/flags/local_flags_provider.rb | Extends initializer to accept credentials and forward into provider config. |
| lib/mixpanel-ruby/flags/flags_provider.rb | Uses credentials for Basic Auth when present; retains token auth fallback. |
| lib/mixpanel-ruby/events.rb | Allows import to accept either API key or service account credentials. |
| lib/mixpanel-ruby/credentials.rb | Implements ServiceAccountCredentials and JSON serialization helpers. |
| lib/mixpanel-ruby/consumer.rb | Sends either service account fields or legacy API key when posting import payloads. |
| lib/mixpanel-ruby.rb | Requires the new credentials file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The stub needs to include the project_id query parameter to match the actual request being made.
Review: public API / backward-compatibility concernsMost of the surface here is additive and compatible ( 1. 🔴
|
|
Hey @lajohn4747 I posted a a Claude review guided by a few concerns I had noticed. Theres a few places where params were added that should probably be added to the end to ensure they aren't breaking. A few other concerns were posted as well. LMK what you think. |
- Add tests for hash-based credentials (string and symbol keys) - Add tests for credential validation error paths - Add test for BufferedConsumer with credentials - Add test for connection error handling - Add test verifying api_key is not sent when credentials are present - Coverage now at 96.85% (was 95.43%)
1. Add validation in Events#import to require authentication - Raises ArgumentError if both api_key and credentials are nil - Prevents confusing silent failures 2. Simplify Tracker#initialize to always call super - Properly initializes parent class in all cases - Overrides sink only when custom consumer provided - Fixes OOP principle violation 3. Add test for import without authentication - Verifies ArgumentError is raised appropriately
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Hash credentials were never used in the actual API flow: - All constructors accept ServiceAccountCredentials or nil - @credentials is always ServiceAccountCredentials or nil - Hash path in Consumer#request was only exercised by tests that directly called request() This removes unnecessary complexity and ~50 lines of dead code. Also add coverage/ and vendor/ to .gitignore to prevent accidental commits.
7ea44e7 to
51974d6
Compare
tylerjroach
left a comment
There was a problem hiding this comment.
No warning when both custom-consumer-block AND credentials are provided. Python logs a warning telling the user credentials will be ignored because they went to Mixpanel() instead of the custom consumer. Ruby silently ignores. Worth adding — 3-line change in Events#initialize:
if block && credentials
warn '[WARNING] credentials passed to Tracker are ignored when a custom sink block is provided. Pass credentials to your consumer directly.'
end
No deprecation warning on legacy api_key path. Python logs a warning on import_data(api_key=...). Ruby doesn't. Greptile flagged this earlier — should be added, once per process to avoid stderr flooding.
|
Update to use a separate method |
Summary