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

Generation support for Microsoft Graph SDK #2721

Closed
wants to merge 6 commits into from
Closed

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Jul 4, 2023

Depends on Importer bug fixes and improvements in #2917


This PR adds support for generating a Microsoft Graph SDK supporting the v1.0 and beta API versions.

  • SDK is located at github.com/hashicorp/go-azure-sdk/microsoft-graph
  • Uses the github.com/hashicorp/go-azure-sdk/sdk/client/msgraph base client
  • Common types, including models, constants, and predicates, are output to the microsoft-graph/common/{version}/models package
  • Services are structured like this:
    Screenshot 2023-09-08 at 02 05 23

Example Client

Screenshot 2023-09-08 at 02 22 42

Example Get Operation

Screenshot 2023-09-08 at 02 06 44

@manicminer manicminer changed the base branch from main to importer-msgraph July 4, 2023 01:29
@manicminer manicminer force-pushed the generator-msgraph branch 2 times, most recently from 5cfa433 to 1534f87 Compare July 11, 2023 09:05
Base automatically changed from importer-msgraph to main August 2, 2023 21:02
@manicminer manicminer changed the base branch from main to bugfix/msgraph-importer-id-detection September 8, 2023 01:02
@manicminer manicminer marked this pull request as ready for review September 8, 2023 01:14
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Left a few comments inline but on the whole this is looking pretty good - nice work 👍

We do need to chat about the rollout of this, we're going to need to make some changes to hashicorp/go-azure-sdk and coordinate that rollout before this is merged (or rather, before Graph SDKs are generated) else we'll hit the Go module limit right away - but once that's done (and the comments are fixed up) - we should be good to get this in, so let's sync up on that soon?

@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Reflection;
using System.Runtime.CompilerServices;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but this isn't technically needed?

Suggested change
using System.Runtime.CompilerServices;

Comment on lines +101 to +102
[JsonPropertyName("referenceIsCommonType")]
public bool? ReferenceIsCommonType { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing this against each API type, given v2 of the Data API exposes per-service level feature-toggles - perhaps we should a) expose and then b) lean on those toggles (e.g. via a new endpoint /v1/(resource-manager|microsoft-graph)/settings)?

Per my understanding at least CommonTypes are supposed to enabled/disabled at a Data Source (e.g. Resource Manager/Microsoft Graph) level - rather than toggled on a per-Service/API Version/Resource level?

}

func (s *CommonTypesGenerator) GenerateForSDK(input SDKInput) error {
input.VersionName = strings.ToLower(input.VersionName)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should have a feature flag in here for UseCommonTypes? Presumably this can be fetched from the Data API (v2) since that has this toggle/information already - whilst that's not in v1 that would be pretty quick to add if needed

@@ -1,6 +1,7 @@
package generator
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw the sdk package has a testhelpers package with a canonical reference for this now

modelNames[*operation.ResponseObject.ReferenceName] = struct{}{}
modelName := *operation.ResponseObject.ReferenceName

if _, ok := data.models[modelName]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

presumably we could output the predicates into the common types directory instead?

@@ -94,7 +94,7 @@ func (p predicateTemplater) templateForModel(predicateStructName string, name st
for _, fieldName := range fieldNames {
fieldVal := model.Fields[fieldName]

typeInfo, err := golangTypeNameForObjectDefinition(fieldVal.ObjectDefinition)
typeInfo, err := golangTypeNameForObjectDefinition(fieldVal.ObjectDefinition, nil) // TODO package path needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

at the time of writing these are scoped to the package, but these might become common types (above) - so a TODO either way

var serviceNames string

f := flag.NewFlagSet("generator-go-sdk", flag.ExitOnError)
f.StringVar(&input.apiServerEndpoint, "data-api", "http://localhost:5000", "-data-api=http://localhost:5000")
f.StringVar(&input.outputDirectory, "output-dir", "", "-output-dir=../generated-sdk-dev")
f.StringVar(&input.outputDirectoryPath, "output-dir", "", "-output-dir=../generated-sdk-dev")
f.StringVar(&input.sdkName, "sdk", "resource-manager", "-sdk=resource-manager")
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw we should look to update the automated tooling to specify this

args := []string{
fmt.Sprintf("-data-api=%s", dataApiUri),
}
if outputDirectory != "" {
args = append(args, fmt.Sprintf("-output-dir=%s", outputDirectory))
}
cmd := exec.Command("generator-go-sdk", args...)

and the scripts: https://github.com/hashicorp/pandora/tree/main/scripts

probably worth adding a resource manager command to the terraform generator at the same time?

args := []string{
"import",
fmt.Sprintf("-data-api=%s", dataApiUri),
}
if outputDirectory != "" {
args = append(args, fmt.Sprintf("-output-dir=%s", outputDirectory))
}
cmd := exec.Command("importer-rest-api-specs", args...)

}
return &version, nil
},
},
}

func main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

kinda outside the scope of this PR, but this is getting complex enough we should probably adopt mitchellh/cli here

Type ApiObjectDefinitionType `json:"type"`
NestedItem *ApiObjectDefinition `json:"nestedItem,omitempty"`
ReferenceName *string `json:"referenceName,omitempty"`
ReferenceIsCommonType *bool `json:"referenceIsCommonType,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) do we need this at this level? if we instead pulled the settings from the API we'd achieve the same thing, which might actually make both generators easier? e.g.

GET /services
{
  "microsoft-graph": {
    "rootUri": "/v1/microsoft-graph",
    "usesCommonTypes": true
  }
  "resource-manager": {
    "rootUri": "/v1/resource-manager",
    "usesCommonTypes": false
  }
}

Which then means the CLI itself could take an argument expecting the Data Source (e.g. Microsoft Graph/Resource Manager) pulled from the API? This has the benefit of features being in one place (but a short-time compile-time hit until we're on v2 of the Data API)

@@ -121,3 +121,16 @@ func GetResourceManagerServicesByName(client resourcemanager.Client, servicesToL
Services: resourceManagerServices,
}, nil
}

// GetResourceManagerCommonTypes returns the specified Services from the Data API
func GetResourceManagerCommonTypes(client resourcemanager.Client) (*ResourceManagerCommonTypes, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(fwiw once the v2 Data API is merged we'll update the SDK to remove the resourcemanager references, presumably having a data-api-sdk folder or similar?)

Base automatically changed from bugfix/msgraph-importer-id-detection to main September 19, 2023 01:29
@hc-github-team-tf-azure
Copy link
Collaborator

No new resource IDs found.

@tombuildsstuff
Copy link
Contributor

x-ref #3601

@tombuildsstuff
Copy link
Contributor

I'm going to closing this PR in favour of #3754, which tracks finishing this support.

At this time the Source Data Type has been threaded through, however the Import Path and Common Types logic hasn't been ported across - but that's tracked in Stage 15 of #3754

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

Successfully merging this pull request may close these issues.

3 participants