Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Generated v1.0 models and request builders using Typewriter#845

Merged
ramsessanchez merged 8 commits intodevmicrosoftgraph/msgraph-sdk-java:devfrom
v1.0/pipelinebuild/54101microsoftgraph/msgraph-sdk-java:v1.0/pipelinebuild/54101Copy head branch name to clipboard
Aug 11, 2021
Merged

Generated v1.0 models and request builders using Typewriter#845
ramsessanchez merged 8 commits intodevmicrosoftgraph/msgraph-sdk-java:devfrom
v1.0/pipelinebuild/54101microsoftgraph/msgraph-sdk-java:v1.0/pipelinebuild/54101Copy head branch name to clipboard

Conversation

@github-actions
Copy link
Contributor

This pull request was automatically created by the GitHub Action, create pull request.

The commit hash is 1932dd7.

Important Check for unexpected deletions or changes in this PR.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

4-2-0_to_java.zip
@zengin @andrueastman @ramsessanchez this update technically introduced a breaking change: the targetApps actions now bind to targetedManagedAppProtection which inherits managedAppProtection instead of managedAppProtection.
Although this would only break people using the request builders directly and not through the fluent API, it's a similar case to the Teams API replacing identitySet by chat message identity set a couple of weeks ago which "forced" us to major rev. See #818
I wanted to get your opinion on this one?

CC @ddyett

@baywet baywet added this to the 4.3.0 milestone Aug 10, 2021
@baywet
Copy link
Member

baywet commented Aug 10, 2021

I took the time to chat about that to @ddyett and his instructions are to major rev same thing for dotnet @andrueastman

@andrueastman
Copy link
Contributor

Thanks for the heads up @baywet

@baywet
Copy link
Member

baywet commented Aug 10, 2021

@ramsessanchez would you mind bumping the major version for this one please?

@ramsessanchez ramsessanchez modified the milestones: 4.3.0, 5.0.0 Aug 10, 2021
@andrueastman
Copy link
Contributor

@baywet, @ramsessanchez
I just noticed that the diff for this PR is may not be consistent with the metadata change.
According to the commit today on the metadata here the targetApps is a newly added action (This is consistent with the .net generation).
Any chance this is what we expect?

ramsessanchez
ramsessanchez previously approved these changes Aug 10, 2021
ramsessanchez
ramsessanchez previously approved these changes Aug 10, 2021
@baywet
Copy link
Member

baywet commented Aug 10, 2021

Thanks for the additional information @andrueastman

If we compare the previous full metadata file with the current one we can see that 2 things happened:

  • a new targetApps action on bound to targetedManagedAppProtection was added
  • a new targetedManagedAppGroupType parameter was added to the targetApps action bound to targetedManagedAppConfiguration

Because targetedManagedAppProtection inherits from managedAppProtection and managedAppProtection already had an action on it, we were generating the request builders and parameter sets for targetedManagedAppProtection as well. (that might only have worked if people were using OData Cast, if the API supports OData cast)
Now that a new action is defined with more precision on the child type, the generation process updated the generated request builder and parameter set.

You can have a look at the pkgdiff, specially at the following files

  • diffs/com/microsoft/graph/requests/TargetedManagedAppProtectionRequestBuilder.class-diff.html
  • diffs/com/microsoft/graph/requests/IosManagedAppProtectionRequestBuilder.class-diff.html
  • (and others)

Now, the chances of anyone hitting these breaking changes are low, but it's still technically a breaking change for Java. And it might be for dotnet, depending on whether or not dotnet generates actions request builders inherited from parent types. Hence the major bump on Java/

Copy link
Contributor

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to explain this @baywet.
I don't think we will need to bump up the major version for dotnet this time as we do not generate action request builders inherited from parent types at moment.
In the dotnet case the change simply shows up as a new method in the "child" request builder class.

@baywet baywet requested a review from ramsessanchez August 11, 2021 15:22
@baywet
Copy link
Member

baywet commented Aug 11, 2021

@ramsessanchez we should be good to merge and release whenever you have time :)

@ramsessanchez ramsessanchez merged commit 7196032 into dev Aug 11, 2021
@ramsessanchez ramsessanchez deleted the v1.0/pipelinebuild/54101 branch August 11, 2021 20:25
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.