Skip to content

chore: Update the azure-rest-api-specs submodule#5474

Open
theunrepentantgeek wants to merge 71 commits into
mainfrom
feature/update-submodule
Open

chore: Update the azure-rest-api-specs submodule#5474
theunrepentantgeek wants to merge 71 commits into
mainfrom
feature/update-submodule

Conversation

@theunrepentantgeek

@theunrepentantgeek theunrepentantgeek commented Jun 16, 2026

Copy link
Copy Markdown
Member

What this PR does

Updates the submodule used to drive our code generator.

This is the first update since January 2026 and there's been a LOT of movement as product teams migrate to typespec internally.

Special notes

We should probably plan to do this monthly in the medium term, so that we don't get further large changes like this one.

How does this PR make you feel?

gif

Checklist

  • this PR contains documentation
  • this PR contains tests

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@theunrepentantgeek theunrepentantgeek force-pushed the feature/update-submodule branch from 000a401 to 20ccb96 Compare June 17, 2026 07:04
@theunrepentantgeek theunrepentantgeek self-assigned this Jun 18, 2026
@theunrepentantgeek theunrepentantgeek added this to the v2.21.0 milestone Jun 18, 2026
@theunrepentantgeek theunrepentantgeek changed the title Update the azure-rest-api-specs submodule chore: Update the azure-rest-api-specs submodule Jun 24, 2026

@matthchr matthchr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Few questions, overall looks great to me

│ ├── Owner: *genruntime.KnownResourceReference
│ ├── Pool: *Object (1 property)
│ ├── Pool: *Object (2 properties)
│ │ ├── AdditionalProperties: map[string]v1.JSON

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would like to understand where this came from and if it's intended/supposed to be here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Same applies to the previous API version)

@theunrepentantgeek theunrepentantgeek Jul 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In commit 6389cc78 we improved our handling for arrays of unknown values, opting to treat them as arrays of any - this flows through into AdditionalProperties showing up in a few new places.

(We needed this because the typespec migrations would otherwise have resulted in a few of our CRDs losing properties they'd already had.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

│ │ └── "Enabled"
│ ├── RedisConfiguration: *Object (17 properties)
│ ├── RedisConfiguration: *Object (18 properties)
│ │ ├── AdditionalProperties: map[string]v1.JSON

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and here

│ ├── TargetResourceId: *string
│ ├── Type: *string
│ └── Weight: *int
├── Endpoints: Object (1 property)[]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did we determine the cause here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was to do with the $resourceEmbeddedInParent config - did I miss fixing one up? I'll check this.

@@ -13,7 +13,8 @@ Workspace: Resource
│ │ └── Rule 2: Pattern: "^[A-Za-z0-9][A-Za-z0-9-]+[A-Za-z0-9]$"
│ ├── DefaultDataCollectionRuleResourceReference: *genruntime.ResourceReference
│ ├── Etag: *string
│ ├── Features: *Object (5 properties)
│ ├── Features: *Object (6 properties)
│ │ ├── AdditionalProperties: map[string]v1.JSON

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar pattern again -- I need to look into what caused it.

Comment thread v2/azure-arm/compute.yaml
SourceVault:
Id:
$referenceType: arm
KeyForDiskEncryptionSet:
KeyUrl:
$importConfigMapMode: optional
EncryptionSetProperties:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Curious: why move?

Alpha order? But if so KeyForDisk... is in the wrong spot too, no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Alphabetical ordering, yes - but I didn't want to create too much chaos by moving all the things, so I just made it closer.

@@ -2,7 +2,8 @@ typeTransformers:
- name: DeploymentProperties
property: CurrentCapacity
remove: true
because: This property should have been marked readonly but wasn't.
because: This property is read-only and cannot be set by the user.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weird changing only this comment?

Comment thread v2/azure-arm/network.yaml
@@ -166,6 +167,8 @@ groupModelConfiguration:
# TODO[breaking]: This is actually a real resource that can be created as a child of a loadbalancer,
# but we didn't model it that way initially, so marking it as not a resource for now to maintain backcompat
$resourceEmbeddedInParent: LoadBalancer
# BackendAddressPool_STATUS_LoadBalancer_SubResourceEmbedded:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Commented out? Intentional? Delete?

WorkspaceFeatures:
ClusterResourceId:
$referenceType: arm
$referenceType: arm

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor: missing newline

@@ -102,7 +103,11 @@ func (scanner *SchemaScanner) AddTypeHandler(schemaType SchemaType, handler Type
}

// RunHandler triggers the appropriate handler for the specified schemaType
func (scanner *SchemaScanner) RunHandler(ctx context.Context, schemaType SchemaType, schema Schema) (astmodel.Type, error) {
func (scanner *SchemaScanner) RunHandler(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These updates are nice but I am curious why they were needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not strictly needed, I made them as I was messing around trying to solve the v1.JSON problem, and then decided to keep them when I committed that fix.

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