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

API Management Service #3368

Merged
merged 18 commits into from
Oct 4, 2023
Merged

Conversation

ross-p-smith
Copy link
Contributor

@ross-p-smith ross-p-smith commented Oct 2, 2023

Relates to #2747

This PR is the first of ~10 that I will add for API Management. This is the root service and the one that takes ~1.5 hours to complete. It is the base API Management. I don't believe that many people will use an operator to build this purely because of the time it takes to complete. However, it is the gateway to the more interesting parts (routes/apis/backends/products, etc)

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

This is really solid. I've flagged a few places where properties need some ASO flavouring, nothing that's difficult to do. Ping us if you have problems getting the recording to play nicely. Once that's included, I'll happily approve.

v2/api/apimanagement/v1api20220801/structure.txt 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
@theunrepentantgeek
Copy link
Member

I have had 1 recording complete, but when re-running it fails.

What error do you have when it fails? It's super likely it's a failure mode we've seen before.

@ross-p-smith
Copy link
Contributor Author

This is really solid. I've flagged a few places where properties need some ASO flavouring, nothing that's difficult to do. Ping us if you have problems getting the recording to play nicely. Once that's included, I'll happily approve.

I've added 2 recordings (1 for the integration and one for samples) - hope that's correct

@ross-p-smith
Copy link
Contributor Author

The previous run worked (before I merged main). It is failing on ServiceBus at the moment

"validate.v1api20221001preview.namespacesauthorizationrules.servicebus.azure.com" denied the request: at least one of ['ARMID'] or ['Group', 'Kind', 'Namespace', 'Name'] must be set for ResourceReference
[produce-markdown-summary]             {
[produce-markdown-summary]                 ErrStatus: {
[produce-markdown-summary]                     TypeMeta: {Kind: "", APIVersion: ""},
[produce-markdown-summary]                     ListMeta: {
[produce-markdown-summary]                         SelfLink: "",
[produce-markdown-summary]                         ResourceVersion: "",
[produce-markdown-summary]                         Continue: "",
[produce-markdown-summary]                         RemainingItemCount: nil,
[produce-markdown-summary]                     },
[produce-markdown-summary]                     Status: "Failure",
[produce-markdown-summary]                     Message: "admission webhook \"validate.v1api20221001preview.namespacesauthorizationrules.servicebus.azure.com\" denied the request: at least one of ['ARMID'] or ['Group', 'Kind', 'Namespace', 'Name'] must be set for ResourceReference",
[produce-markdown-summary]                     Reason: "Forbidden",
[produce-markdown-summary]                     Details: nil,
[produce-markdown-summary]                     Code: 403,
[produce-markdown-summary]                 },
[produce-markdown-summary]             }
[produce-markdown-summary] --- FAIL: Test_ServiceBus_Namespace_Standard_v1api20221001preview_CRUD/subtests/AuthorizationRule_CRUD (0.02s)

@theunrepentantgeek
Copy link
Member

That servicebus error doesn't seem related to your changes. I've merged main so we can see if it recurrs. I'll mention it to the other members of the ASO team to see if they've seen it.

@theunrepentantgeek
Copy link
Member

/ok-to-test sha=027aa84

v2/api/apimanagement/v1api20220801/structure.txt Outdated Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt Outdated Show resolved Hide resolved
v2/azure-arm.yaml Show resolved Hide resolved
v2/api/apimanagement/v1api20220801/structure.txt Outdated Show resolved Hide resolved
@matthchr
Copy link
Member

matthchr commented Oct 4, 2023

/ok-to-test sha=e7f9e1c

@theunrepentantgeek
Copy link
Member

That servicebus error doesn't seem related to your changes. I've merged main so we can see if it recurrs. I'll mention it to the other members of the ASO team to see if they've seen it.

@matthchr has found and fixed the problem in PR #3373; once you've merged main into your branch things should run smoothly.

@theunrepentantgeek
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theunrepentantgeek
Copy link
Member

Looks good to me!

@theunrepentantgeek theunrepentantgeek merged commit 2e2d223 into Azure:main Oct 4, 2023
9 checks passed
@ross-p-smith ross-p-smith deleted the ross/apim_service branch October 4, 2023 08:56
│ │ │ │ ├── SubjectFromConfig: *genruntime.ConfigMapReference
│ │ │ │ ├── Thumbprint: *string
│ │ │ │ └── ThumbprintFromConfig: *genruntime.ConfigMapReference
│ │ │ ├── CertificatePassword: *string
Copy link
Member

Choose a reason for hiding this comment

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

Hmm it seems this is still getting exposed as a string?

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, there's 2 one here and one below.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #3377

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants