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

Fix non-azure openai generation issues - part 1 #2041

Merged
merged 15 commits into from
Oct 11, 2023

Conversation

MaryGao
Copy link
Member

@MaryGao MaryGao commented Oct 8, 2023

this is part of the issue #2042 and fixes #2046.

  • Support cutom http auth in Modular and enable auth testing in cadl-ranch
  • Fix error response issue not from azure core
  • Fallback to package name if no scope defined in package name
  • Add a pipeline property in classical client to allow customizing policy

@MaryGao MaryGao changed the title Fix the error model generation issue not in azure-core Fix non-azure openai generation issues Oct 8, 2023
@MaryGao MaryGao changed the title Fix non-azure openai generation issues Fix non-azure openai generation issues - part 1 Oct 8, 2023
@MaryGao MaryGao marked this pull request as ready for review October 9, 2023 07:05
import { assert } from "chai";
import { customBearerTokenAuthenticationPolicy } from "../util/customBearerTokenTestingPolicy.js";

describe("OAuth2Context in API Layer", () => {
Copy link
Member Author

@MaryGao MaryGao Oct 9, 2023

Choose a reason for hiding this comment

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

We can only change underlying policy in API layer and can't do this in classical layer. This is a breaking for mgmt SDK, I was wondering if we need to re-visit this design.

@qiaozha How do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Based on our past experience when handling the customer issues for mgmt plane, we do recommend our customers to use customized policy for serveral times. Personally, I don't think it worth a breaking here to recommend them to custom it in the api layer.

If we want to have an exact same user experience like mgmt plane to use client.pipeline.addPolicy() and client.pipeline.removePolicy() , we have to add a new property in the classical client layer.
public pipeline: Pipeline; and change the constructor like

export class ApiKeyClient {
  private _client: ApiKeyContext;
  public pipeline: Pipeline;

  /** Illustrates clients generated with ApiKey authentication. */
  constructor(credential: KeyCredential, options: ApiKeyClientOptions = {}) {
    this._client = createApiKey(credential, options);
    this.pipeline = this._client.pipeline;
  }
  ...
}

Not sure if others have different opinions or have other options about it. @joheredi thoughts ?

Copy link
Member Author

@MaryGao MaryGao Oct 10, 2023

Choose a reason for hiding this comment

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

I prefer to have a pipeline in classical client first and we could refactor these logic to core whenever necessary in future. @joheredi Not sure you have any concern?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in commit ab2632b

Copy link
Member

Choose a reason for hiding this comment

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

why not change the test into using the classical client layer pipeline ?

Copy link
Member Author

Choose a reason for hiding this comment

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

describe("UnionClient in classical client", () => {

@MaryGao MaryGao merged commit b3a5345 into Azure:main Oct 11, 2023
28 checks passed
@MaryGao
Copy link
Member Author

MaryGao commented Oct 11, 2023

@joheredi I merged this pr but feel free to add any comment if you have!

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.

Fix custom http auth and other small issues
2 participants