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.

Sign up for GitHub

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

Jump to bottom

Improve readme "Get started" and fix credential policy issue. #1976

Closed
wants to merge 2 commits into from
Closed

Improve readme "Get started" and fix credential policy issue. #1976

wants to merge 2 commits into from

Conversation

lukben2000
Copy link

No description provided.

@lukben2000
Update README.md with correct way to specify input-file when running …
31fd186 
…`autorest --typescript`

The previous stated approach did not work and you need to use the `--input-file=<swagger file>` flag to set the input file.
@lukben2000
Use the options.credential in the added credential policy.
f9c5a72 
`credential` that was referrered to previously is undefined.
@microsoft-github-policy-service microsoft-github-policy-service bot added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Aug 16, 2023
@microsoft-github-policy-service
Copy link
Contributor

Thank you for your contribution @lukben2000! We will review the pull request and get back to you soon.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Community Contribution Community members are working on the issue label Aug 16, 2023
@lukben2000
Copy link
Author

@lukben2000 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

MaryGao
MaryGao reviewed Aug 17, 2023
View reviewed changes
packages/autorest.typescript/src/generators/clientFileGenerator.ts
@@ -710,7 +710,7 @@ function getTrack2DefaultContent(
});
this.pipeline.addPolicy(
coreRestPipeline.bearerTokenAuthenticationPolicy({
credential: credentials,
credential: options.credential,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add more descriptions for this fix?

My understanding is that we enable this policy only when there is a required parameter credentials so we should not go to optional one options.credential.

Copy link
Author

Choose a reason for hiding this comment

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

I had to do a search-and-replace temp fix for my project, when using this package. In the previous implementation credentials are not defined. So it might be a matter of exposing this variable then, but let me just have a second look when I get off work

Copy link
Author

Choose a reason for hiding this comment

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

I am running autorest with the following command:
autorest --typescript --package-name=api-client --input-file=api-specs.json --package-version=1.0.0 --add-credentials=true --generate-metadata=true --generate-client-as-impl=true --generate-client-interfaces --output-folder=api-client-package

Maybe I am missing something in regards to the credentials?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what kind of issue you met, if you enable the option add-credentials it's better to define your credential details in OpenAPI(refer link), otherwise we will met the known issue and I was trying to fix in this pr.

Please file a new one if your case is different https://github.com/Azure/autorest.typescript/issues.

@qiaozha qiaozha added the needs-author-feedback More information is needed from author to address the issue. label Sep 13, 2023
@MaryGao
Copy link
Contributor

MaryGao commented Sep 14, 2023

@lukben2000 Could you try wiht our latest version https://www.npmjs.com/package/@autorest/typescript/v/6.0.9? I believe this should be fixed in our latest.

I will close this pr considering it's fixed and feel free to re-open it if still existing.

@MaryGao MaryGao closed this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaryGao MaryGao MaryGao left review comments

@sarangan12 sarangan12 Awaiting requested review from sarangan12 sarangan12 is a code owner

@joheredi joheredi Awaiting requested review from joheredi joheredi is a code owner

@qiaozha qiaozha Awaiting requested review from qiaozha qiaozha is a code owner

Assignees
No one assigned
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-author-feedback More information is needed from author to address the issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lukben2000 @MaryGao @qiaozha