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

Authorization resource for APIM #3644

Merged
merged 22 commits into from
Feb 10, 2024
Merged

Authorization resource for APIM #3644

merged 22 commits into from
Feb 10, 2024

Conversation

ansulgoenka
Copy link
Contributor

Closes #[issue number]

What this PR does / why we need it:
It adds authorization resource for APIM
Special notes for your reviewer:

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

@ansulgoenka ansulgoenka marked this pull request as ready for review December 14, 2023 02:36
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023 •
edited
Loading

Codecov Report

Attention: 1817 lines in your changes are missing coverage. Please review.

Comparison is base (cac58a1) 53.52% compared to head (0775d12) 53.45%.

Files Patch % Lines
.../v1api20220801/authorization_provider_types_gen.go 50.52% 230 Missing and 51 partials ⚠️
...0230501preview/authorization_provider_types_gen.go 54.69% 184 Missing and 62 partials ⚠️
...review/storage/authorization_provider_types_gen.go 48.13% 149 Missing and 60 partials ⚠️
...authorization_providers_authorization_types_gen.go 54.43% 161 Missing and 29 partials ⚠️
...authorization_providers_authorization_types_gen.go 57.94% 132 Missing and 40 partials ⚠️
...roviders_authorizations_access_policy_types_gen.go 50.00% 143 Missing and 26 partials ⚠️
...roviders_authorizations_access_policy_types_gen.go 52.70% 127 Missing and 39 partials ⚠️
...authorization_providers_authorization_types_gen.go 43.95% 119 Missing and 34 partials ⚠️
...roviders_authorizations_access_policy_types_gen.go 50.19% 105 Missing and 24 partials ⚠️
...220801/storage/authorization_provider_types_gen.go 56.66% 24 Missing and 2 partials ⚠️
... and 8 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3644      +/-   ##
==========================================
- Coverage   53.52%   53.45%   -0.08%     
==========================================
  Files        1419     1440      +21     
  Lines      485724   489635    +3911     
==========================================
+ Hits       259999   261717    +1718     
- Misses     186428   188226    +1798     
- Partials    39297    39692     +395

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tomkerkhove
tomkerkhove reviewed Dec 14, 2023
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

A few notes - primarily because I believe this will only be in v2.6 since 2.4 is already released

docs/hugo/content/reference/_index.md Outdated Show resolved Hide resolved
docs/hugo/content/reference/_index.md Outdated Show resolved Hide resolved
docs/hugo/content/reference/apimanagement/_index.md Outdated Show resolved Hide resolved
docs/hugo/content/reference/apimanagement/_index.md Outdated Show resolved Hide resolved
v2/azure-arm.yaml Outdated Show resolved Hide resolved
v2/samples/apimanagement/v1api20220801/v1api20220801_authorizationprovider.yaml Outdated Show resolved Hide resolved
super-harsh
super-harsh reviewed Dec 15, 2023
View reviewed changes
Copy link
Collaborator

@super-harsh super-harsh left a comment

Choose a reason for hiding this comment

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

PR looks good. Just a few minor suggestions below.
Also, we'll need #3435 to be resolved before we can support AuthorizationProvider resource as it has collection of secrets which we don't support yet.

v2/azure-arm.yaml Outdated Show resolved Hide resolved
v2/azure-arm.yaml Outdated Show resolved Hide resolved
v2/azure-arm.yaml Outdated Show resolved Hide resolved
v2/internal/controllers/crd_apimanagement_20220801_test.go Outdated Show resolved Hide resolved
v2/azure-arm.yaml Outdated Show resolved Hide resolved
v2/azure-arm.yaml Outdated Show resolved Hide resolved
v2/internal/controllers/crd_apimanagement_20220801_test.go Outdated Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

@ansulgoenka can you please also support 2023-05-01-preview please?

@tomkerkhove
Copy link
Member

tomkerkhove commented Jan 22, 2024 •
edited
Loading

Re-posting here to ensure we don't forget: #3644 (comment) and is tracked in #3435

AuthorizationCode and ClientCredentials should be secret properties

@ansulgoenka Can you please add "Depends on #3435" in the PR comment please to clarify this?

matthchr
matthchr reviewed Feb 1, 2024
View reviewed changes
Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! I've left some relatively minor comments. It looks pretty good overall.

v2/README.md Outdated Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt Outdated Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt Outdated Show resolved Hide resolved
v2/internal/controllers/recordings/Test_ApiManagement_20230501preview_CRUD.yaml Outdated Show resolved Hide resolved
v2/samples/apimanagement/v1api20220801/v1api20220801_authorizationprovidersauthorization.yaml Outdated Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt Outdated Show resolved Hide resolved
...anagement/v1api20230501preview/v1api20230501preview_authorizationprovidersauthorization.yaml Outdated Show resolved Hide resolved
tomkerkhove
tomkerkhove approved these changes Feb 8, 2024
View reviewed changes
v2/api/apimanagement/v1api20220801/storage/structure.txt Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt
Comment on lines +988 to +989
│ │ │ ├── AuthorizationCode: map[string]string
│ │ │ └── ClientCredentials: map[string]string
Copy link
Member

Choose a reason for hiding this comment

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

Same here

v2/api/apimanagement/v1api20220801/structure.txt
Comment on lines +999 to +1000
│ │ ├── AuthorizationCode: map[string]string
│ │ └── ClientCredentials: map[string]string
Copy link
Member

Choose a reason for hiding this comment

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

Same here

v2/api/apimanagement/v1api20230501preview/storage/structure.txt Show resolved Hide resolved
@matthchr
Copy link
Member

matthchr commented Feb 9, 2024

/ok-to-test sha=0775d12

@matthchr
Copy link
Member

matthchr commented Feb 9, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matthchr matthchr added this pull request to the merge queue Feb 10, 2024
Merged via the queue into Azure:main with commit c5c478d Feb 10, 2024
9 checks passed
@theunrepentantgeek theunrepentantgeek added this to the v2.6.0 milestone Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ross-p-smith ross-p-smith ross-p-smith left review comments

@super-harsh super-harsh super-harsh left review comments

@tomkerkhove tomkerkhove tomkerkhove approved these changes

@matthchr matthchr matthchr approved these changes

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
Azure Service Operator Roadmap
Archived in project
Milestone
v2.6.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ansulgoenka @codecov-commenter @tomkerkhove @matthchr @theunrepentantgeek @ross-p-smith @super-harsh