Conversation
- Introduced a new configuration file `incloud.yaml` for global settings. - Enhanced the `runDeployCmd` function to support fetching an ingestion key if no API key is provided, with a fallback to the regular API key. - Updated `selectBackendName` to accept a tenant UUID instead of a tenant object. - Refactored the ingestion key command to utilize the new API client for key management, allowing for custom names for ingestion keys.
noamApps
requested review from
AvitalTamir,
avivzgroundcover,
maxlevin-gc and
yair-sedaka
WalkthroughThe deployment and ingestion key logic was refactored to unify API key retrieval via a new helper that tries multiple sources, including a new sensor ingestion key fetched through an API client method. Backend name selection was updated to use tenant UUID and return InCloud status. The ingestion key command now uses the API client’s new method. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant APIClient
participant SDKClient
User->>CLI: Run deploy command
CLI->>APIClient: selectBackendName(tenantUUID, deployFlow)
APIClient-->>CLI: backendName, isInCloud
CLI->>APIClient: GetOrCreateIngestionKey(tenantUUID, backendName, ingestionKeyType, customName)
APIClient->>SDKClient: ListIngestionKeys(filter by name)
alt Key exists
SDKClient-->>APIClient: Existing key
APIClient-->>CLI: Ingestion key
else No key
APIClient->>SDKClient: CreateIngestionKey(name, type)
SDKClient-->>APIClient: New key
APIClient-->>CLI: Ingestion key
end
CLI->>CLI: Continue deployment with resolved API key
sequenceDiagram
participant User
participant CLI
participant APIClient
User->>CLI: Run get-ingestion-key command
CLI->>APIClient: GetOrCreateIngestionKey(tenantUUID, backendName, ingestionKeyType, customName)
APIClient-->>CLI: Ingestion key (existing or new)
CLI-->>User: Output key
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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 (4)
cmd/deploy.go (2)
163-180: Smart fallback logic but improve error messagingThe ingestion-key-first approach with fallback is well-designed. However, the error message on line 169 should be more consistent with CLI conventions.
- ui.GlobalWriter.PrintlnWithPrefixln("ingestion key not found, falling back to regular API key") + ui.GlobalWriter.PrintlnWithPrefixln("Ingestion key not found, falling back to regular API key")
693-710: Consider Auth0 token reuse optimizationThe function loads Auth0 token again, but the caller likely already has one. Consider passing the token as parameter to avoid redundant operations.
-func fetchIngestionKey(tenantUUID, backendName string) (string, error) { +func fetchIngestionKey(tenantUUID, backendName string, auth0Token *auth.Auth0Token) (string, error) { var err error - var auth0Token *auth.Auth0Token - if auth0Token, err = auth.LoadAuth0Token(); err != nil { - return "", err - } - apiClient := api.NewClient(auth0Token)cmd/ingestion_key.go (1)
36-55: Excellent refactoring - much cleanerThe new API client approach eliminates manual SDK operations and improves readability. The argument parsing is functional but could be more robust.
Consider using more descriptive error messages:
- return fmt.Errorf("ingestion key type is required") + return fmt.Errorf("ingestion key type is required: specify one of [sensor|rum|thirdParty]")pkg/api/client.go (1)
210-260: Well-structured method but consider optimizationThe method encapsulates ingestion key logic effectively. However, loading Auth0 token again is redundant since the client was created with a token.
Consider accessing the token from the client's transport:
-// GetOrCreateIngestionKey retrieves an existing CLI ingestion key or creates a new one -func (client *Client) GetOrCreateIngestionKey(tenantUUID, backendName, ingestionKeyType, customName string) (string, error) { - // Create SDK client - auth0Token, err := auth.LoadAuth0Token() - if err != nil { - return "", err - } +func (client *Client) GetOrCreateIngestionKey(tenantUUID, backendName, ingestionKeyType, customName string) (string, error) { + // Extract token from client transport + transport := client.httpClient.Transport.(*TransportWithAuth0Token) + auth0Token := transport.auth0Token
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/deploy.go(3 hunks)cmd/get_datasources_api_key.go(1 hunks)cmd/ingestion_key.go(2 hunks)cmd/login.go(2 hunks)pkg/api/client.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
cmd/deploy.go (5)
cmd/root.go (1)
API_KEY_FLAG(29-29)pkg/ui/writer.go (1)
GlobalWriter(37-37)pkg/auth/api_key.go (1)
ApiKey(13-15)pkg/auth/auth0.go (2)
Auth0Token(20-25)LoadAuth0Token(39-59)pkg/api/client.go (1)
NewClient(41-55)
cmd/ingestion_key.go (1)
pkg/api/client.go (1)
NewClient(41-55)
pkg/api/client.go (3)
pkg/auth/client.go (1)
Client(21-27)pkg/auth/auth0.go (1)
LoadAuth0Token(39-59)pkg/client/client.go (1)
NewDefaultClient(63-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
cmd/get_datasources_api_key.go (1)
22-22: LGTM - Consistent with refactoring patternChange aligns with updated
selectBackendNamesignature across codebase.cmd/login.go (2)
135-135: Good interface improvementFunction now takes only what it needs (UUID) instead of entire tenant object.
145-145: Consistent with signature changeBackend list call properly uses tenant UUID parameter.
cmd/deploy.go (1)
158-161: Good refactoring - backend name selection moved earlierMoving backend selection before API key fetching enables reuse in ingestion key logic.
cmd/ingestion_key.go (2)
18-18: Updated example reflects new optional parameterGood documentation update for the new custom name functionality.
32-32: Consistent with refactoring patternProperly uses tenant UUID with updated
selectBackendNamesignature.pkg/api/client.go (2)
4-5: New imports support ingestion key functionalityRequired imports for context, string manipulation, and SDK client operations.
Also applies to: 9-9, 11-12, 14-14
17-17: Reasonable naming conventionThe constant provides consistent naming for CLI-generated keys.
AvitalTamir
left a comment
AvitalTamir
left a comment
There was a problem hiding this comment.
@noamApps this looks good to me - except the valid CodeRabbit comment there, I don't know if I fully understand the context - but it seems like the backendName variable is being repurposed and is expected to get 2 different values during this process?
If it is for a different purpose and is expected to get a completely new value - why not rename the first one to something else like backendNameFromTenant (don't know if this makes sense)?
If it is not expected to get a new value, why overwrite it?
If there's a chance it's empty - why no assign it to either selectBackendName(tenantUUID) or clusterName to begin with?
Other than that looks cool! clarify or fix and I'll approve 🙏
- Updated `selectBackendName` to return a boolean indicating if the backend is in the cloud and modified its signature to accept a deploy flow parameter. - Refactored `runDeployCmd` to streamline API key retrieval, utilizing a new `getApiKey` function for improved clarity and maintainability. - Introduced `getComponentsConfiguration` to encapsulate logic for determining agent and backend configurations based on chart values. - Adjusted related commands to align with the updated backend selection logic.
AvitalTamir
left a comment
AvitalTamir
left a comment
There was a problem hiding this comment.
Approved with comment, take it or leave it
|
Sorry, my bad I missed this
…On Wed, 23 Jul 2025 at 20:12 Noam Levy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In cmd/deploy.go
<#319 (comment)>:
> @@ -197,33 +187,25 @@ func runDeployCmd(cmd *cobra.Command, args []string) error {
chartValues = release.Config
}
- if chartValues, err = generateChartValues(chartValues, apiKey, installationId, clusterName, deployableNodes, tolerations, nodesReport, sentryHelmContext); err != nil {
+ var backendName string
not sure I follow, there is no initial value now , its now
backendName=clusterName only if no backend name is discovered (line 683)
if backendName == "" {
backendName = clusterName
}
if isIncloud {
return true, false, backendName
}
—
Reply to this email directly, view it on GitHub
<#319 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AT2ZLTNDGB35ZTXBHRO7SM33J666DAVCNFSM6AAAAACBQBAV6CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTANBYGM2DGOBZGA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
- Split `getComponentsConfiguration` into `getAgentComponentsConfiguration` and `getBackendComponentsConfiguration` for clearer separation of concerns. - Updated `runDeployCmd` to utilize the new configuration functions, enhancing readability and maintainability. - Simplified the logic for determining agent and backend configurations based on chart values.
noamApps
deleted the
noam/core-5674-cli-incloud-deploy-uses-ingestion-key-custom-key-name
branch
3 participants
Summary by CodeRabbit
New Features
Refactor
Documentation