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

[SDK Automation] azure-sdk-for-java generate script build error #9222

Closed
JackTn opened this issue Oct 21, 2024 · 16 comments
Closed

[SDK Automation] azure-sdk-for-java generate script build error #9222

JackTn opened this issue Oct 21, 2024 · 16 comments
Assignees

Comments

@JackTn
Copy link
Member

JackTn commented Oct 21, 2024

After run azure-sdk-for-java Generate Script in sdk-automation.
It missing changelog content and version when input file is relatedTypeSpecProjectFolder or relatedReadmeMdFiles. The pipeline result you can see 1 2
The correct response refer to this

@raych1
Copy link
Member

raych1 commented Oct 21, 2024

@weidongxu-microsoft @XiaofeiCao
both content and breakingChangeItems need to be added to report the breaking changes.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Oct 22, 2024

@raych1

I did not get it. There is only 1 stable api-version in AzureFleet (computefleet as SDK)
https://github.com/Azure/azure-rest-api-specs/tree/main/specification/azurefleet/resource-manager/Microsoft.AzureFleet
And Java hadn't released it (it is scheduled today -- 10/22).

So, there is no breaking change to report at all -- there is no previous GA SDK, every SDK before is beta/preview

SDK only concerns about breaking change compared to a prior GA SDK (it be 2nd GA compares to 1st GA; or a next beta compares to 1st GA -- but you have to have a 1st GA SDK)

@JackTn
Copy link
Member Author

JackTn commented Oct 22, 2024

@weidongxu-microsoft Apologies for any confusion caused. The issue at hand is that the breakingChangeItems are not being returned as expected. For a clearer understanding, please refer to the results shown here result. and here is correct response this.Additionally, it appears that the version has been returned accurately. Therefore, you may disregard it.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Oct 22, 2024

@JackTn

I still didn't get you.

Result in azure-data-schemaregistry is because there was GA release 1.5.0 https://azure.github.io/azure-sdk/releases/latest/?search=azure-data-schemaregistry

No result for computefleet and datafactory is because there were no GA release in Java. (computefleet released 1.0.0 today -- that was after your pipeline) https://azure.github.io/azure-sdk/releases/latest/?search=azure-resourcemanager-datafactory

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Oct 22, 2024

And DO NOT COMPARE JAVA WITH GO. Breaking change depends on SDK state of each language, and they could be in different state.

Java SDK cares about whether some change could break a GA Java SDK. No concern about go SDK.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Oct 22, 2024

You can reopen with a PR in specs, that it contains breaks to a released/GAed Java SDK, but it didn't have content in breaking changes section.

If you are unsure whether Java code change would be a break, check Revapi step in Java CI. https://dev.azure.com/azure-sdk/public/_build/results?buildId=4225865&view=logs&jobId=b70e5e73-bbb6-5567-0939-8415943fadb9&j=b70e5e73-bbb6-5567-0939-8415943fadb9&t=ba3cf43a-b2bf-50f9-f95f-93049ca44ad1

@JackTn
Copy link
Member Author

JackTn commented Oct 22, 2024

Thansks, will check on this CI

@JackTn
Copy link
Member Author

JackTn commented Oct 24, 2024

@raych1

I did not get it. There is only 1 stable api-version in AzureFleet (computefleet as SDK) https://github.com/Azure/azure-rest-api-specs/tree/main/specification/azurefleet/resource-manager/Microsoft.AzureFleet And Java hadn't released it (it is scheduled today -- 10/22).

So, there is no breaking change to report at all -- there is no previous GA SDK, every SDK before is beta/preview

SDK only concerns about breaking change compared to a prior GA SDK (it be 2nd GA compares to 1st GA; or a next beta compares to 1st GA -- but you have to have a 1st GA SDK)

As the AzureFleet is release, this pr has correct hasBreakingChange flag for JAVA. But why there are no breakingchange items return when the hasBreakingChange is true ? @weidongxu-microsoft

@JackTn JackTn reopened this Oct 24, 2024
@github-project-automation github-project-automation bot moved this from Done to Backlog in API to SDK automation Oct 24, 2024
@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Oct 24, 2024

@XiaofeiCao to take a look

@JackTn Which "breakingchange items" is it? This https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/sdkautomation/GenerateOutputSchema.json#L73-L79 ? I guess Xiaofei didn't fill this one. It's added pretty late in Spec.

@XiaofeiCao
Copy link

@JackTn Seems this breakingChangeItems is added after we supported breaking change detection, see original issue #8477 (comment)

I'll add support for this property

@JackTn
Copy link
Member Author

JackTn commented Oct 24, 2024

@JackTn Seems this breakingChangeItems is added after we supported breaking change detection, see original issue #8477 (comment)

I'll add support for this property

Thanks!

@XiaofeiCao
Copy link

XiaofeiCao commented Oct 29, 2024

We have roughly three kinds of breaking changes

1. class-level

#### `ModelA` was removed

proposed breaking change item:

- Class `ModelA` was removed.

2. method level

#### `ModelB` was modified
* `sku()` was deleted
* `tier()` was deleted

proposed breaking change item:

- Method `sku()` was deleted in class `ModelB`.
- Method `tier()` was deleted in class `ModelB`.

3. fluent lite stage

#### `ModelC$DefinitionStage` was modified
* `properties()` was deleted from stage 3

proposed breaking change item:

- Method `properties()` was deleted from stage 3 in class `ModelC$DefinitionStage`.

Alternatives

a. Combine class and method changes

For method level breaking changes, we could combine them into one item for each class.
e.g. for 2:

- Class `ModelB` was modified: `sku()` was deleted, `tier()` was deleted.

Benefit would be easier to add suppression, though will lose some granularity in suppression. And when method changes are large, the line could grow very long.

b. Leave it as is

e.g. for 2:

- `ModelB` was modified\n* `sku()` was deleted\n* `tier()` was deleted

This is a bit hard to read.

c. Remove class level info

e.g. for 2:

- `sku()` was deleted
- `tier()` was deleted

benefit would be, when sku is removed from multiple classes, it's easier to batch suppress.
though it'll also lose granularity, and a bit vague to track

@raych1
Copy link
Member

raych1 commented Oct 29, 2024

@XiaofeiCao how about the other cases, such as adding new required parameters, changing the optional parameters to required parameters? Are they considered as breaking changes?

@XiaofeiCao
Copy link

XiaofeiCao commented Oct 30, 2024

@XiaofeiCao how about the other cases, such as adding new required parameters, changing the optional parameters to required parameters? Are they considered as breaking changes?

Yeah, adding a new required parameters(or changing optional to required) will result in method signature change. This is method-level breaking change in case 2.

@raych1
Copy link
Member

raych1 commented Nov 19, 2024

@JackTn
can you please verify and close this issue if it has been fixed?

@JackTn JackTn closed this as completed Dec 10, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in API to SDK automation Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

4 participants