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

add auth into plugin generation #5209

Merged
merged 26 commits into from
Aug 30, 2024
Merged

Conversation

calebkiage
Copy link
Contributor

@calebkiage calebkiage commented Aug 22, 2024

See #5070

allow falling back to the global security if the operation has none.
# Conflicts:
#	src/Kiota.Builder/Plugins/PluginsGenerationService.cs
baywet
baywet previously approved these changes Aug 27, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

@calebkiage calebkiage marked this pull request as ready for review August 27, 2024 16:05
@calebkiage calebkiage requested a review from a team as a code owner August 27, 2024 16:05
baywet
baywet previously approved these changes Aug 27, 2024
@calebkiage
Copy link
Contributor Author

@baywet, the test is failing for 1 of the ACs, because of a gap in OpenAPI. I'll open an issue in the OpenAPI.NET repo.

Copy link
Member

@andrueastman andrueastman 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 from my end.

You'll need to run dotnet format for the linting in the CI to pass.
Could we also add a changelog entry for this change as well?

baywet
baywet previously approved these changes Aug 28, 2024
# Conflicts:
#	CHANGELOG.md
#	src/Kiota.Builder/Configuration/GenerationConfiguration.cs
#	tests/Kiota.Builder.Tests/Plugins/PluginsGenerationServiceTests.cs
@calebkiage calebkiage disabled auto-merge August 30, 2024 12:08
@calebkiage
Copy link
Contributor Author

Note for reviewers:

The feature to use global security information if local operation security is not provided is waiting on resolution of microsoft/OpenAPI.NET#1797

@calebkiage
Copy link
Contributor Author

calebkiage commented Aug 30, 2024

@baywet, I think the logic of StringIEnumerableDeepComparer might be problematic. ["a", "b"] and ["b", "a"] have the same hash code. Won't this pessimize collection performance?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
79.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

@baywet
Copy link
Member

baywet commented Aug 30, 2024

@baywet, I think the logic of StringIEnumerableDeepComparer might be problematic. ["a", "b"] and ["b", "a"] have the same hash code. Won't this pessimize collection performance?

for others reading, we discussed that internally. And a better implementation would be to instead use HashCode. This is a broader change and we don't want to delay the plugins auth for that though. Caleb will create a separate PR to address it across the board.

@calebkiage calebkiage merged commit 76fa86f into main Aug 30, 2024
208 of 209 checks passed
@calebkiage calebkiage deleted the caleb/feat/plugin-manifest-security branch August 30, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

If the security scheme is provided, Kiota should use it to create the right auth object
3 participants