Skip to content

Conversation

b4s36t4
Copy link
Contributor

@b4s36t4 b4s36t4 commented Aug 25, 2025

Description

Allows azure foundry api calls to be authenticated using Azure Entra or Managed Identity

Motivation

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

Copy link
Contributor

matter-code-review bot commented Aug 25, 2025

Code Quality bug fix Refactoring Performance

Description

Summary By MatterAI MatterAI logo

🔄 What Changed

This pull request refactors the handling of fetchPortkey's return value in plugins/portkey/gibberish.ts, plugins/portkey/language.ts, and plugins/portkey/moderateContent.ts by introducing destructuring assignment for the response property. Additionally, the Dockerfile has been updated to remove the apk update command from two RUN instructions, aiming to streamline the build process.

🔍 Impact of the Change

The destructuring assignment improves code clarity and aligns with a potential change in the fetchPortkey function's return signature, making the code more robust and readable. The Dockerfile modification may lead to slightly faster image builds but could potentially result in using older package versions if the apk upgrade command does not implicitly refresh the package index.

📁 Total Files Changed

  • Dockerfile: Removed apk update from two RUN commands.
  • plugins/portkey/gibberish.ts: Modified fetchPortkey call to destructure the response property.
  • plugins/portkey/language.ts: Modified fetchPortkey call to destructure the response property as result.
  • plugins/portkey/moderateContent.ts: Modified fetchPortkey call to destructure the response property as result.

🧪 Test Added

N/A

🔒Security Vulnerabilities

N/A

Motivation

This change refactors how the return value of fetchPortkey is handled across multiple plugins for improved code clarity and adapts to potential API changes. It also optimizes Docker image build times by removing an unnecessary apk update command.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

N/A

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

N/A
Total Checklist Score: 5

Tip

Quality Recommendations

  1. Reconsider removing apk update in the Dockerfile to ensure the latest package lists are fetched before upgrade, preventing potential issues with outdated dependencies.

  2. Ensure the fetchPortkey function's return type is explicitly defined to reflect the object structure, improving type safety and maintainability.

Tanka Poem ♫

Docker's swift build,
apk update now departs,
Code's new structure gleams,
Response, a named treasure,
Cleaned and ready, logic flows. 🚀✨

Sequence Diagram

sequenceDiagram
    participant PH as PluginHandler (e.g., gibberish.ts)
    participant FP as fetchPortkey
    participant PK_API as Portkey API

    PH->>FP: fetchPortkey(options.env, PORTKEY_ENDPOINTS.GIBBERISH, parameters.credentials, ...)
    Note over FP: Calls external Portkey API
    FP->>PK_API: API Request (e.g., POST /portkey/gibberish)
    PK_API-->>FP: API Response (object with 'response' property)
    FP-->>PH: Returned Object { response: ..., status: ... }
    Note over PH: Destructures { response } or { response: result } from object
Loading

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

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

Adding Azure authentication provider options - code looks clean with consistent naming patterns.

@VisargD VisargD merged commit da07600 into Portkey-AI:main Aug 25, 2025
1 check passed
Copy link
Contributor

Important

PR Review Skipped

PR review skipped as per the configuration setting. Run a manually review by commenting /matter review

💡Tips to use Matter AI

Command List

  • /matter summary: Generate AI Summary for the PR
  • /matter review: Generate AI Reviews for the latest commit in the PR
  • /matter review-full: Generate AI Reviews for the complete PR
  • /matter release-notes: Generate AI release-notes for the PR
  • /matter : Chat with your PR with Matter AI Agent
  • /matter remember : Generate AI memories for the PR
  • /matter explain: Get an explanation of the PR
  • /matter help: Show the list of available commands and documentation
  • Need help? Join our Discord server: https://discord.gg/fJU5DvanU3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants