CLOUDP-345026: Update the L1 generation tool to support x-xgen-operation-id-override #4233
Conversation
Luke-Sanderson
changed the title
| if overrides := extractOverrides(operation.Extensions); overrides != nil { | ||
| if overriddenOperationID, ok := overrides["operationId"].(string); ok && overriddenOperationID != "" { | ||
| operationID = overriddenOperationID | ||
| shortOperationID = "" |
There was a problem hiding this comment.
nit: Wondering could we just not set extensions.shortOperationID in func processShortOperationIDOverride instead of here since we are already doing some logic for if an atlasCLI opId override exists (referring to here)
There was a problem hiding this comment.
Removed this check due to it being a duplicate
| OperationID: `listIdentityProviders`, | ||
| ShortOperationID: ``, | ||
| Aliases: nil, |
There was a problem hiding this comment.
I think the spec needs to be updated. Looking at the latest openapi, "x-xgen-operation-id-override": "listIdentityProviders"
I expect here to see:
OperationID: `listFederationSettingIdentityProviders `,
ShortOperationID: `listIdentityProviders`,
Aliases: []string{`listFederationSettingIdentityProviders`},
(see this make command. This github workflow may also help)
There was a problem hiding this comment.
I think this will be covered by CLOUDP-347637 (Failed github workflow run for context).
We might need to merge the other ticket in before this one?
| operationID = overriddenOperationID | ||
| shortOperationID = "" | ||
| } | ||
| if shortOperationID != "" { |
There was a problem hiding this comment.
Do we need this logic? Since we already to the same thing in cli/api?
There was a problem hiding this comment.
We do not and I have now removed it
Luke-Sanderson
changed the base branch from
master
to
op-id-override-feature-branch
Luke-Sanderson
changed the base branch from
op-id-override-feature-branch
to
master
| return nil, fmt.Errorf("failed to clean description: %w", err) | ||
| } | ||
|
|
||
| if overrides := extractOverrides(operation.Extensions); overrides != nil { |
There was a problem hiding this comment.
[q] for my own understanding, we have removed operationId overrides officially in benefit of x-xgen-operation-id-override? I'm trying to understand if we haven't missed any overrides here that should've been translated to x-xgen-operation-id-override
There was a problem hiding this comment.
There is no mention of replacing the operationId override officially by x-xgen-operation-id-override in the TD so I think I removed this my mistake. Will add it back in
There was a problem hiding this comment.
but does it make sense to keep two types of overrides? I think my main concern is just making sure we're handling both if they exist, but we could look into how the overrides are being added at the moment and if we should rename them, or maybe, we don't even use them
There was a problem hiding this comment.
Since both target the same field (operationID) one will override the other. From what I can see in the latest openapi spec, I don't think operationID override is used. It was appear as x-operationid right?
There was a problem hiding this comment.
interesting! I think this is the AtlasCLI framework for overrides, e.g. search for the openapi spec tests with x-xgen-atlascli. There is a different format there for different overrides. Perharps by keeping this here, the idea is that if we need to override via AtlasCLI we can, let's drop a comment here maybe capturing this behaviour in case we see this in the future?
There was a problem hiding this comment.
Oh yes this is the x-xgen-atlascli one. I think I might have originally removed it because I thought it was duplicating the logic from here. Yes I agree there is a use case for this override for overriding the operationId only for atlascli and I think that this override should take priority.
I will add a comment to reflect this
4 participants
Proposed changes
Jira ticket: CLOUDP-345026
contributing.mdChecklist
make fmtand formatted my codeFurther comments