Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add temporal API key #1028

Merged
merged 4 commits into from
Jan 8, 2025
Merged

feat: Add temporal API key #1028

merged 4 commits into from
Jan 8, 2025

Conversation

whiterabbit1983
Copy link
Contributor

@whiterabbit1983 whiterabbit1983 commented Jan 8, 2025

PR Type

Enhancement


Description

  • Added support for Temporal API key in client configuration.

  • Introduced temporal_api_key environment variable in env.py.

  • Updated docker-compose.yml to include TEMPORAL_API_KEY.

  • Enhanced Temporal client connection with optional API key.


Changes walkthrough 📝

Relevant files
Enhancement
temporal.py
Add Temporal API key to client connection                               

agents-api/agents_api/clients/temporal.py

  • Added api_key parameter to Temporal client connection.
  • Integrated temporal_api_key into the client setup.
  • +2/-0     
    Configuration changes
    env.py
    Introduce `temporal_api_key` environment variable               

    agents-api/agents_api/env.py

  • Added temporal_api_key as a new environment variable.
  • Default value for temporal_api_key set to None.
  • +1/-0     
    docker-compose.yml
    Add `TEMPORAL_API_KEY` to docker-compose configuration     

    agents-api/docker-compose.yml

  • Added TEMPORAL_API_KEY to shared environment variables.
  • Default value for TEMPORAL_API_KEY set to an empty string.
  • +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information


    Important

    Add Temporal API key support to client configuration and environment variables in temporal.py, env.py, and docker-compose.yml.

    • Behavior:
      • Add api_key parameter to get_client() and get_client_with_metrics() in temporal.py using temporal_api_key.
    • Environment Variables:
      • Add temporal_api_key to env.py for environment variable parsing.
      • Add TEMPORAL_API_KEY to docker-compose.yml for Docker configuration.

    This description was created by Ellipsis for 8029c00. It will automatically update as commits are pushed.

    @whiterabbit1983 whiterabbit1983 marked this pull request as ready for review January 8, 2025 06:18
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The TEMPORAL_API_KEY is being added as an environment variable. While the default is empty, care should be taken to ensure this API key is properly secured in production environments and not logged or exposed in any way. Consider using a secrets management system for production deployments.

    ⚡ Recommended focus areas for review

    Error Handling

    The code should handle potential authentication errors when using the API key for Temporal client connection

    api_key=temporal_api_key or None,

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Reviewed everything up to 776832e in 14 seconds

    More details
    • Looked at 46 lines of code in 3 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/clients/temporal.py:46
    • Draft comment:
      Consider adding api_key=temporal_api_key or None to the get_client_with_metrics function to ensure consistent authentication across all client connections.
    • Reason this comment was not posted:
      Comment was not on a valid diff hunk.

    Workflow ID: wflow_k4AzEKYTNoPcNwtW


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    qodo-merge-pro-for-open-source bot commented Jan 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation to prevent empty string API keys from being used for authentication

    Add input validation to ensure the API key is not an empty string when provided, as
    this could cause authentication issues with Temporal.

    agents-api/agents_api/clients/temporal.py [49]

    -api_key=temporal_api_key or None,
    +api_key=temporal_api_key if temporal_api_key and temporal_api_key.strip() else None,
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential security and functionality issue by preventing empty strings from being treated as valid API keys, which could cause authentication failures or unexpected behavior.

    8
    General
    Automatically clean environment variable input to prevent whitespace-related authentication issues

    Use env.str() with strip=True to automatically remove whitespace from the API key
    environment variable, preventing accidental whitespace-only values.

    agents-api/agents_api/env.py [102]

    -temporal_api_key: str = env.str("TEMPORAL_API_KEY", default=None)
    +temporal_api_key: str = env.str("TEMPORAL_API_KEY", default=None, strip=True)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding strip=True to env.str() is a robust way to handle whitespace in environment variables, preventing potential authentication issues caused by accidental whitespace in the API key.

    7

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on 57b515c in 11 seconds

    More details
    • Looked at 54 lines of code in 3 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/clients/temporal.py:49
    • Draft comment:
      The use of temporal_api_key or None is redundant since temporal_api_key is already set to None by default in env.py. Consider using api_key=temporal_api_key directly here and on line 81.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The use of temporal_api_key or None is redundant since temporal_api_key is already set to None by default in env.py. This pattern is repeated in two places in temporal.py.

    Workflow ID: wflow_vHLT0fQQNeiVKmDB


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    ❌ Changes requested. Incremental review on 6bed999 in 38 seconds

    More details
    • Looked at 54 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 0 drafted comments based on config settings.

    Workflow ID: wflow_c4ABswa5czGdceEX


    Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Copy link
    Contributor

    CI Failure Feedback 🧐

    Action: Lint-And-Format

    Failed stage: Run stefanzweifel/git-auto-commit-action@v4 [❌]

    Failure summary:

    The action failed because Git was unable to push changes to the remote repository. Specifically:

  • The branch f/temporal-api-key is behind its remote counterpart (non-fast-forward error)
  • Git rejected the push because local branch needs to be updated with remote changes first
  • The error suggests running git pull to integrate remote changes before pushing again

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    1297:  * [new branch]      f/embed-instruction     -> origin/f/embed-instruction
    1298:  * [new branch]      f/entries-summarization -> origin/f/entries-summarization
    1299:  * [new branch]      f/grafana-traefik       -> origin/f/grafana-traefik
    1300:  * [new branch]      f/increase-test-coverage -> origin/f/increase-test-coverage
    1301:  * [new branch]      f/integrations-sentry   -> origin/f/integrations-sentry
    1302:  * [new branch]      f/prompt-step-run-tools -> origin/f/prompt-step-run-tools
    1303:  * [new branch]      f/remote-browser-args-connect-url -> origin/f/remote-browser-args-connect-url
    1304:  * [new branch]      f/scispace-poc          -> origin/f/scispace-poc
    1305:  * [new branch]      f/tasks-validation-errors-enhancement -> origin/f/tasks-validation-errors-enhancement
    1306:  * [new branch]      main                    -> origin/main
    1307:  * [new branch]      mintlify-docs           -> origin/mintlify-docs
    1308:  * [new branch]      translate-readme        -> origin/translate-readme
    1309:  * [new branch]      v0.3                    -> origin/v0.3
    1310:  * [new branch]      x/error-transition      -> origin/x/error-transition
    ...
    
    1324:  [f/temporal-api-key ba7fa29] refactor: Lint agents-api (CI)
    1325:  Author: whiterabbit1983 <[email protected]>
    1326:  1 file changed, 2 insertions(+), 2 deletions(-)
    1327:  INPUT_TAGGING_MESSAGE: 
    1328:  No tagging message supplied. No tag will be added.
    1329:  INPUT_PUSH_OPTIONS: 
    1330:  To https://github.com/julep-ai/julep
    1331:  ! [rejected]        f/temporal-api-key -> f/temporal-api-key (non-fast-forward)
    1332:  error: failed to push some refs to 'https://github.com/julep-ai/julep'
    1333:  hint: Updates were rejected because the tip of your current branch is behind
    1334:  hint: its remote counterpart. If you want to integrate the remote changes,
    1335:  hint: use 'git pull' before pushing again.
    1336:  hint: See the 'Note about fast-forwards' in 'git push --help' for details.
    1337:  Error: Invalid status code: 1
    1338:  at ChildProcess.<anonymous> (/home/runner/work/_actions/stefanzweifel/git-auto-commit-action/v4/index.js:17:19)
    1339:  at ChildProcess.emit (node:events:519:28)
    1340:  at maybeClose (node:internal/child_process:1105:16)
    1341:  at ChildProcess._handle.onexit (node:internal/child_process:305:5) {
    1342:  code: 1
    1343:  }
    1344:  Error: Invalid status code: 1
    ...
    
    1350:  [command]/usr/bin/git version
    1351:  git version 2.47.1
    1352:  Temporarily overriding HOME='/home/runner/work/_temp/cf8c175b-0fea-462b-9fc2-d8b0a68cc3b5' before making global git config changes
    1353:  Adding repository directory to the temporary git global config as a safe directory
    1354:  [command]/usr/bin/git config --global --add safe.directory /home/runner/work/julep/julep
    1355:  [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
    1356:  [command]/usr/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
    1357:  fatal: No url found for submodule path 'drafts/cozo' in .gitmodules
    1358:  ##[warning]The process '/usr/bin/git' failed with exit code 128
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on 8029c00 in 21 seconds

    More details
    • Looked at 22 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 1 drafted comments based on config settings.
    1. agents-api/agents_api/clients/temporal.py:46
    • Draft comment:
      Duplicate assignment of rpc_metadata. Remove the redundant rpc_metadata = in both get_client and get_client_with_metrics functions.
    • Reason this comment was not posted:
      Comment looked like it was already resolved.

    Workflow ID: wflow_VpT45dfKySEanSgg


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    @whiterabbit1983 whiterabbit1983 merged commit 544f5f8 into dev Jan 8, 2025
    12 of 14 checks passed
    @whiterabbit1983 whiterabbit1983 deleted the f/temporal-api-key branch January 8, 2025 08:27
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant