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

DevCenter @ 2022-04-01 #2675

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Conversation

jiaweitao001
Copy link
Contributor

@tombuildsstuff
Copy link
Contributor

x-link: hashicorp/go-azure-sdk#549

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Jul 26, 2023

Looks like there's a Swagger issue blocking this one:

2023-06-21T08:45:22.664Z [INFO]  Importer for Service "DevCenter".Importer for API Version "2023-04-01": ❌ Service "DevCenter" - Api Version "2023-04-01"
Error: -21T08:45:22.664Z [ERROR] Importer for Service "DevCenter".Importer for API Version "2023-04-01":      💥 Error: parsing Swagger files: parsing files in "../../swagger/specification/devcenter/resource-manager/Microsoft.DevCenter/stable/2023-04-01": parsing definition: finding resources for tag "Environment Types": finding nested items yet to be parsed: appending model: appending models: different model objects. First fields: map[roles:{Required:false ReadOnly:false Sensitive:false JsonName:roles Description:A map of roles to assign to the environment creator. CustomFieldType:<nil> ObjectDefinition:0xc0001c4fc0}]. Second fields: map[creatorRoleAssignment:{Required:false ReadOnly:false Sensitive:false JsonName:creatorRoleAssignment Description:The role definition assigned to the environment creator on backing resources. CustomFieldType:<nil> ObjectDefinition:0xc0001c5d40} deploymentTargetId:{Required:false ReadOnly:false Sensitive:false JsonName:deploymentTargetId Description:Id of a subscription that the environment type will be mapped to. The environment's resources will be deployed into this subscription. CustomFieldType:<nil> ObjectDefinition:0xc0001c5b80} status:{Required:false ReadOnly:false Sensitive:false JsonName:status Description:Defines whether this Environment Type can be used in this Project. CustomFieldType:<nil> ObjectDefinition:0xc0001c5c00} userRoleAssignments:{Required:false ReadOnly:false Sensitive:false JsonName:userRoleAssignments Description:Role Assignments created on environment backing resources. This is a mapping from a user object ID to an object of role definition IDs. CustomFieldType:<nil> ObjectDefinition:0xc0001c5f00}]
2023/06/21 08:45:22 Error: parsing data for Service "DevCenter": parsing data for Service "DevCenter" / Version "2023-04-01": parsing Swagger files: parsing files in "../../swagger/specification/devcenter/resource-manager/Microsoft.DevCenter/stable/2023-04-01": parsing definition: finding resources for tag "Environment Types": finding nested items yet to be parsed: appending model: appending models: different model objects. First fields: map[roles:{Required:false ReadOnly:false Sensitive:false JsonName:roles Description:A map of roles to assign to the environment creator. CustomFieldType:<nil> ObjectDefinition:0xc0001c4fc0}]. Second fields: map[creatorRoleAssignment:{Required:false ReadOnly:false Sensitive:false JsonName:creatorRoleAssignment Description:The role definition assigned to the environment creator on backing resources. CustomFieldType:<nil> ObjectDefinition:0xc0001c5d40} deploymentTargetId:{Required:false ReadOnly:false Sensitive:false JsonName:deploymentTargetId Description:Id of a subscription that the environment type will be mapped to. The environment's resources will be deployed into this subscription. CustomFieldType:<nil> ObjectDefinition:0xc0001c5b80} status:{Required:false ReadOnly:false Sensitive:false JsonName:status Description:Defines whether this Environment Type can be used in this Project. CustomFieldType:<nil> ObjectDefinition:0xc0001c5c00} userRoleAssignments:{Required:false ReadOnly:false Sensitive:false JsonName:userRoleAssignments Description:Role Assignments created on environment backing resources. This is a mapping from a user object ID to an object of role definition IDs. CustomFieldType:<nil> ObjectDefinition:0xc0001c5f00}]

Rebased this on top of main and still present.

@tombuildsstuff tombuildsstuff added data/swagger-issue An issue related to the Swagger/OpenAPI Definitions requires-investigation labels Jul 26, 2023
@tombuildsstuff tombuildsstuff changed the title Add devcenter 2022-04-01 DevCenter @ 2022-04-01 Jul 26, 2023
@am-lim
Copy link

am-lim commented Aug 16, 2023

@tombuildsstuff I am from the team owning the swagger. Do you have any suggestions on how to fix this issue? Thank you.

@tombuildsstuff
Copy link
Contributor

@am-lim looking into this one, it appears the root of the issue is that the EnvironmentType model is duplicated, once using the in-lined approach (in creatorRoleAssignment) and another by reference (UserRoleAssignment), which is unexpected/should be able to be parsed.

The following diff fixes this issue:

diff --git a/specification/devcenter/resource-manager/Microsoft.DevCenter/stable/2023-04-01/devcenter.json b/specification/devcenter/resource-manager/Microsoft.DevCenter/stable/2023-04-01/devcenter.json
index e0b943d07a..424114f64d 100644
--- a/specification/devcenter/resource-manager/Microsoft.DevCenter/stable/2023-04-01/devcenter.json
+++ b/specification/devcenter/resource-manager/Microsoft.DevCenter/stable/2023-04-01/devcenter.json
@@ -3406,15 +3406,11 @@
         "creatorRoleAssignment": {
           "description": "The role definition assigned to the environment creator on backing resources.",
           "type": "object",
-          "properties": {
-            "roles": {
-              "type": "object",
-              "description": "A map of roles to assign to the environment creator.",
-              "additionalProperties": {
-                "$ref": "#/definitions/EnvironmentRole"
-              }
+          "allOf": [
+            {
+              "$ref": "#/definitions/RoleAssignment"
             }
-          }
+          ]
         },
         "userRoleAssignments": {
           "description": "Role Assignments created on environment backing resources. This is a mapping from a user object ID to an object of role definition IDs.",
@@ -3674,10 +3670,8 @@
         }
       }
     },
-    "UserRoleAssignment": {
+    "RoleAssignment": {
       "type": "object",
-      "description": "Mapping of user object ID to role assignments.",
-      "x-ms-client-name": "userRoleAssignmentValue",
       "properties": {
         "roles": {
           "type": "object",
@@ -3688,6 +3682,16 @@
         }
       }
     },
+    "UserRoleAssignment": {
+      "type": "object",
+      "description": "Mapping of user object ID to role assignments.",
+      "x-ms-client-name": "userRoleAssignmentValue",
+      "allOf": [
+        {
+          "$ref": "#/definitions/RoleAssignment"
+        }
+      ]
+    },
     "OperationStatus": {
       "description": "The current status of an async operation",
       "type": "object",

However the import should be able to process this as it is, so I believe this is ultimately a bug in the importer which needs fixing.

@am-lim
Copy link

am-lim commented Aug 16, 2023

Thanks for the suggestion @tombuildsstuff. Is it possible that the bug in the importer will be fixed soon?

@am-lim
Copy link

am-lim commented Aug 31, 2023

@tombuildsstuff The suggestion would cause a breaking change on our side which would involve difficulty in getting our SDK updated in the meanwhile and immediate future. Could we prioritize the importer bug to be fixed?

tombuildsstuff added a commit that referenced this pull request Sep 4, 2023
@am-lim
Copy link

am-lim commented Sep 6, 2023

@tombuildsstuff Thanks for fixing the bug! Any update on this PR? Can the checks be re-run?

@tombuildsstuff
Copy link
Contributor

@am-lim FWIW the bug isn't fixed from our side, I've added a test for this in the branch above whilst I have the context, but as there's some higher priority items from our side, so unfortunately I don't have a timeframe for when this'll be fixed in the importer.

That said, if @jiaweitao001 or someone else is able to send a PR to fix that in the interim then we'd be happy to accept a PR for that - but in the short-term the most reliable way to fix this is going to be to update the Swagger (which, fwiw, I don't believe this'd be a breaking change, since the SDK output is the same?)

wuxu92 pushed a commit to wuxu92/pandora that referenced this pull request Sep 14, 2023
@tombuildsstuff tombuildsstuff removed data/swagger-issue An issue related to the Swagger/OpenAPI Definitions requires-investigation labels Sep 22, 2023
@tombuildsstuff
Copy link
Contributor

@jiaweitao001 @wuxu92 mind rebasing this one so we can get this in?

@jiaweitao001
Copy link
Contributor Author

@tombuildsstuff Thanks for reminding. Rebased.

@am-lim
Copy link

am-lim commented Oct 2, 2023

@tombuildsstuff Any update on whether this PR can be checked in?

@tombuildsstuff tombuildsstuff merged commit 92dfae3 into hashicorp:main Oct 4, 2023
2 checks passed
@tombuildsstuff
Copy link
Contributor

@am-lim @jiaweitao001 looks like there's an issue with the Swagger here where this field isn't marked as readonly: Azure/azure-rest-api-specs#26189 - mind taking a look so we can add support for this?

@am-lim
Copy link

am-lim commented Oct 12, 2023

@tombuildsstuff the change looks good, verified by @nickdepinet

@am-lim
Copy link

am-lim commented Oct 19, 2023

@tombuildsstuff The /Azure/azure-rest-api-specs/pull/26189 has been merged

@tombuildsstuff
Copy link
Contributor

Awesome, thanks @am-lim :)

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

Successfully merging this pull request may close these issues.

3 participants