Introducing profileBuilder and rewriting generator #767
Conversation
glide.lock
Outdated
| ======= | ||
| hash: fc11c273a4bfae9a077f2238a4693fa3af3d815dfc1b1e9ec75d9eb8e6428993 | ||
| updated: 2017-05-23T10:47:59.5894489-07:00 | ||
| >>>>>>> c9acadb... Fixing glide defs to include Masterminds/semver |
There was a problem hiding this comment.
Whoops whoops whoops, I'll get this cleaned up.
tools/generator/program.go
Outdated
|
|
||
| const ( | ||
| defaultRemoteAzureRestAPISpecsPath = "https://github.com/Azure/azure-rest-api-specs.git" | ||
| defaultAzureRESTAPIBranch = "master" |
There was a problem hiding this comment.
master [](start = 39, length = 6)
The branch we use is "current"
| } | ||
|
|
||
| func cloneAPISpecs(origin, local string) (string, error) { | ||
| _, cloneLoc := filepath.Split(local) |
There was a problem hiding this comment.
Autorest does not need the repo to be cloned on local. You can do autorest [https://raw.githubusercontent.com/Azure/azure-rest-api-specs/current/specification/graphrbac/data-plane/readme.md](https://raw.githubusercontent.com/Azure/azure-rest-api-specs/current/specification/graphrbac/data-plane/readme.md%60).
There was a problem hiding this comment.
Ah, this is a new developement, that's great :)
There was a problem hiding this comment.
I'll leave this in here for now though, I suspect I'm going to have to make a bunch of changes to accomodate the new Autorest work.
| } | ||
|
|
||
| fmt.Println("Summary:") | ||
| fmt.Println("\tFound: \t", found) |
There was a problem hiding this comment.
Would be useful to also go build packages, I think.
| "-OutputDirectory", outputDir, | ||
| "-Modeler", "Swagger", | ||
| "-pv", version.String(), | ||
| "-SkipValidation") |
There was a problem hiding this comment.
This is from the legacy autorest CLI
| `[Mm]icrosoft\.(?P<category>[\w\-]+)`, | ||
| `(?P<apiVersion>v?\d{4}-\d{2}-\d{2}(?:[\-\.][\w\d\-\.]+)?)`, | ||
| `(?P<group>[\w\-\d\.]+)\.json`, | ||
| } |
There was a problem hiding this comment.
Will you support md files as input?
tools/profileBuilder/latest.go
Outdated
| // AcceptAll is a predefined value for `LatestStrategy.Predicate` which always returns true. | ||
| func AcceptAll(name string) (result bool) { | ||
| result = true | ||
| return |
There was a problem hiding this comment.
I would prefer to keep it simple and return true
There was a problem hiding this comment.
I'll make this change because I like you and Joel. 😝
tools/profileBuilder/latest.go
Outdated
| // IgnorePreview searches a packages "API Version" to see if it contains the word "preview". It only returns true when a package is not a preview. | ||
| func IgnorePreview(name string) (result bool) { | ||
| matches := packageName.FindStringSubmatch(name) | ||
| if len(matches) >= 4 { |
There was a problem hiding this comment.
= 4 [](start = 17, length = 4)
These number look a bit magical, add a comment explaining a little bit.
There was a problem hiding this comment.
You're telling me you don't appreciate the magic in life? Sure thing, I'll add some documentation here.
I was actually quite frustrated with Go's Regexp library, because in C# you can reference groups by the name you gave them instead of some index. The way goes about it, it's quite prone to these sorts of magic numbers.
| } | ||
|
|
||
| var versionle = func() func(string, string) (bool, error) { | ||
| versionPattern := regexp.MustCompile(`^(?P<year>\d{4})-(?P<month>\d{2})-(?P<day>\d{2})(?:[\.\-](?P<tag>.+))?$`) |
There was a problem hiding this comment.
versionPattern [](start = 1, length = 14)
Graphrbac uses a different pattern
There was a problem hiding this comment.
I think the amount of work for this versus the reward warrants fixing this in a separate PR.
tools/profileBuilder/program.go
Outdated
| outputLog.Print("Profile Name Set to: ", profileName) | ||
| } | ||
|
|
||
| // |
There was a problem hiding this comment.
// [](start = 1, length = 2)
O.O
| go get -d github.com/Azure/azure-sdk-for-go/tools/profileBuilder | ||
| cd $GOPATH/src/github.com/Azure/azure-sdk-for-go/tools/profileBuilder | ||
| glide install | ||
| export currentCommit=$(git rev-parse HEAD) |
There was a problem hiding this comment.
git rev-parse HEAD [](start = 23, length = 18)
Could this be executed inside the profile builder, and just have a bool flag for including or not the commit?
There was a problem hiding this comment.
Actually, it cannot be done this way :) That would result in the profileBuilder always telling you it was the most up-to-date version even if it wasn't. We really need to set this variable at build time the way I've done it.
- Generator now uses TEMP directory to write logs of the results. - Autorest is called from the correct directory.
The tools/profileBuilder package will traverse services/ directory in search of the most recent API Versions of individual Operation Groups. Optionally, this operation will include preview versions. It will also be able to be configured to read the individual packages required from a list to create a custom profile.
Also, adding Apache 2.0 headers to each of the files touched here and writing installation instructions.
This is laying the foundation to move to the Multiple API Version and Profile future of our API. Once this is accepted, I'll send another PR with the generated new packages including each API Version and profile.
You can see examples of what the generated code looks like here:
https://github.com/Azure/azure-sdk-for-go/tree/experimental/allAPIVersions