Skip to content

Comments

Generate Managers and POMs for Azure SDK for Java#2446

Closed
RikkiGibson wants to merge 9 commits intoAzure:masterfrom
RikkiGibson:azure-gen-manager
Closed

Generate Managers and POMs for Azure SDK for Java#2446
RikkiGibson wants to merge 9 commits intoAzure:masterfrom
RikkiGibson:azure-gen-manager

Conversation

@RikkiGibson
Copy link
Member

Adds some templates to generate stub Manager classes. The generation of the Manager and POM is controlled by the RegenerateManager flag. Note that the path provided to Autorest in the Azure SDK for Java project has changed so that we can reasonably write the POM to the correct location.

The ServiceName property is something that we just expect to work most of the time due to the convention for paths in Azure, as well as the fact that we expect each service we generate code for to have at least one method.

@azuresdkci
Copy link

Can one of the admins verify this patch?

@msftclas
Copy link

@RikkiGibson,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@jianghaolu
Copy link
Contributor

@azuresdkci add to whitelist

<parent>
<groupId>com.microsoft.azure</groupId>
<artifactId>azure-parent</artifactId>
<version>1.1.3-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this version can go to the code generator? Same as the Beta annotation in the manager template.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can get the beta annotation version from this version though - 1.x.0-SNAPSHOT can generate @beta(SinceVersion=V1_X_0) and 1.x.y-SNAPSHOT can generate @beta(SinceVersion=V1_{X+1}_0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Would we pass a value from CodeGeneratorJvaf to the template indicating which version of Azure SDK for Java is being targeted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - that would be ideal

Copy link
Contributor

Choose a reason for hiding this comment

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

One day we can do a cmd argument but I think we can do that later

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is that the generated manager class will be able to be merged into master without further editing.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the input/output you'd like to see? Someone passes -AzureSDKVersion 1.1.3, then the pom gets 1.1.3-SNAPSHOT and the SinceVersion is V1_2_0?

@jianghaolu
Copy link
Contributor

Seems like you need to run gulp regenerate to reflect the changes for the tests.

@RikkiGibson
Copy link
Member Author

Will need to troubleshoot my gulp and node setup to get gulp regenerate to run without exploding.

@fearthecowboy
Copy link
Contributor

fearthecowboy commented Jul 18, 2017 via email

@RikkiGibson
Copy link
Member Author

Thank you. I took the steps you both mentioned and now I seem to be regenerating fine.

@RikkiGibson
Copy link
Member Author

@jianghaolu Seems like about 880 files were changed through regeneration--should I be committing all of that? Or just picking out Java.Azure.Fluent related stuff?

@fearthecowboy
Copy link
Contributor

fearthecowboy commented Jul 18, 2017 via email

@RikkiGibson
Copy link
Member Author

I'm seeing a lot of:

- // Code generated by Microsoft (R) AutoRest Code Generator 1.1.0.0
+ // Code generated by Microsoft (R) AutoRest Code Generator 1.2.0.0

@fearthecowboy
Copy link
Contributor

fearthecowboy commented Jul 18, 2017 via email

var versionParts = targetVersion.Split('.');
var minorVersion = int.Parse(versionParts[1]) + 1;
var result = "V" + versionParts[0] + "_" + minorVersion + "_0";
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

one more nit picking - 2.0.0-SNAPSHOT will give me V2_1_0 but it should be V2_0_0 - you need to check the last digit to see if it's 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching that. Should there be unit tests for this? Or we'll just run it and see if it did the right thing?

@RikkiGibson
Copy link
Member Author

I merged in master and regenerated again. Now I'm here:

- // Code generated by Microsoft (R) AutoRest Code Generator 1.2.1.0
+ // Code generated by Microsoft (R) AutoRest Code Generator 1.2.2.0

@fearthecowboy
Copy link
Contributor

Well, that's more like it -- although not unexpected. I guess we forgot to regenerated when we bumped the version number.

Go ahead and add those changes into your PR.

@RikkiGibson
Copy link
Member Author

Did you want me to open a new PR, @fearthecowboy?

@fearthecowboy fearthecowboy reopened this Jul 18, 2017
@msftclas
Copy link

@RikkiGibson,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@fearthecowboy
Copy link
Contributor

Whoops. hit wrong button.

This constant should become a parameter at some point anyway.
@jianghaolu
Copy link
Contributor

@RikkiGibson I think there's still quite some diff - http://azuresdkci.cloudapp.net/job/autorest/5692/console

@RikkiGibson
Copy link
Member Author

@jianghaolu I'm not sure how to interpret the output from CI. Did I get something wrong when regenerating sources?

@fearthecowboy
Copy link
Contributor

@olydis -- we're seeing this variation again in the CI with generating source maps.

O_o

@olydis
Copy link
Contributor

olydis commented Jul 19, 2017

Seems to depend on weird details... maybe it's non-deterministic. Let's just not compare source maps in CI for now, even though there are differences they don't seem crucial.

@RikkiGibson
Copy link
Member Author

Should I be doing anything to make that happen? Or will you guys go in and tweak that after this PR?

@fearthecowboy
Copy link
Contributor

He's fixing the issue in the 2.0 branch -- You're going to need to rebase and issue a new PR there, but after that gets committed.

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.

6 participants