chore: add @InternalApi to binary compatibility checks; introduce new PR check to verify no API-breaking changes #1030
Conversation
… PR check to verify no API-breaking changes
ianbotsf
added
the
acknowledge-api-break
ianbotsf
added
no-changelog
|
|
||
| on: | ||
| pull_request: | ||
| types: [ opened, synchronize, reopened, labeled, unlabeled ] |
There was a problem hiding this comment.
Is there any reason to specify these types? Our other workflows look like they apply to all types
There was a problem hiding this comment.
I copied the job definition from changelog-verification.yml which also explicitly specifies PR events. I think, in general, we could be more conservative in the subtypes of events which trigger action runs and reduce some of the churn/cost associated with them.
Do you have a reason to prefer running on all event subtypes?
There was a problem hiding this comment.
No actually, I think the current types cover all the cases where this should run, so it's good to leave it as is. I just never saw these types specified (I did not look at the changelog-verification.yml job) and was curious
There was a problem hiding this comment.
In practice for us these are likely about the same but it's fine as is, here is the full list for reference if you leave it unspecified and just have pull_request:
| if: ${{ failure() }} | ||
| run: | | ||
| echo "::error ::This change modifies the public API in a way that may be backwards-incompatible. Carefully" | ||
| echo "::error ::review this pull request and either:" |
There was a problem hiding this comment.
style: It might be better to combine L29 and L30 because GitHub's CI output autowraps the text
(I think it looks good on one line)
|
| @@ -116,8 +116,6 @@ configureLinting(lintPaths) | |||
|
|
|||
| // Binary compatibility | |||
| apiValidation { | |||
| nonPublicMarkers.add("aws.smithy.kotlin.runtime.InternalApi") | |||
There was a problem hiding this comment.
In general I agree with this change but I am wondering about what we will do when we do need/want to break internal APIs. Presumably we'd deprecate the APIs and leave them in place for some period of time or perhaps indefinitely and switch codegen to use whatever the replacement is?
There was a problem hiding this comment.
Discussed offline and decided largely on what @aajtodd suggested: if we need to deprecate an @InternalApi declaration, we'll mark the API as @Deprecated and leave it in place for some number of releases. It will be considered for (potentially-breaking) removal if it is determined to be actively harming something (e.g., security, critical bugs, etc.).
Issue #
awslabs/aws-sdk-kotlin#1191
Description of changes
This change tightens backwards compatibility guarantees by:
@InternalApifrom the exemptions for the backwards compatibility verifierBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.