chore: add @InternalApi to binary compatibility checks; introduce new PR check to verify no API-breaking changes #1030
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| name: API compatibility verification | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [ opened, synchronize, reopened, labeled, unlabeled ] | ||
| branches: [ main ] | ||
|
|
||
| jobs: | ||
| api-compat-verification: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - name: Check for API compatibility | ||
| if: ${{ !contains(github.event.pull_request.labels.*.name, 'acknowledge-api-break') }} | ||
| run: | | ||
| git fetch origin ${{ github.base_ref }} --depth 1 && \ | ||
| git diff remotes/origin/${{ github.base_ref }} --numstat "*.api" | awk ' | ||
| BEGIN { s = 0 } | ||
|
|
||
| # git diff numstat shows lines deleted in field 2, hence sum up field 2 across all items | ||
| { s += $2 } | ||
|
|
||
| # exit with the number of lines deleted as the result code so that `if failure()` works below | ||
| END { exit s } | ||
| ' | ||
| - name: Error message | ||
| if: ${{ failure() }} | ||
| run: | | ||
| echo "::error ::This change modifies the public API in a way that may be backwards-incompatible. Carefully review this pull request and either:" | ||
| echo "::error ::* Revert the changes which caused the API incompatibility –or–" | ||
| echo "::error ::* Add the 'acknowledge-api-break' label to this PR (in rare cases warranting an API breakage)" | ||
| exit 1 | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,8 +116,6 @@ configureLinting(lintPaths) | |
|
|
||
| // Binary compatibility | ||
| apiValidation { | ||
|
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 |
||
| ignoredProjects.addAll( | ||
| setOf( | ||
| "dokka-smithy", | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,31 @@ | ||
| public final class aws/smithy/kotlin/runtime/crt/HttpKt { | ||
| public static final fun path (Laws/sdk/kotlin/crt/http/HttpRequest;)Ljava/lang/String; | ||
| public static final fun queryParameters (Laws/sdk/kotlin/crt/http/HttpRequest;)Laws/smithy/kotlin/runtime/net/url/QueryParameters; | ||
| public static final fun toCrtHeaders (Laws/smithy/kotlin/runtime/http/Headers;)Laws/sdk/kotlin/crt/http/Headers; | ||
| public static final fun toSdkHeaders (Laws/sdk/kotlin/crt/http/Headers;)Laws/smithy/kotlin/runtime/http/Headers; | ||
| public static final fun toSignableCrtRequest (Laws/smithy/kotlin/runtime/http/request/HttpRequest;ZZLkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public static synthetic fun toSignableCrtRequest$default (Laws/smithy/kotlin/runtime/http/request/HttpRequest;ZZLkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object; | ||
| public static final fun update (Laws/smithy/kotlin/runtime/http/request/HttpRequestBuilder;Laws/sdk/kotlin/crt/http/HttpRequest;)V | ||
| } | ||
|
|
||
| public final class aws/smithy/kotlin/runtime/crt/ReadChannelBodyStream : aws/sdk/kotlin/crt/http/HttpRequestBodyStream, kotlinx/coroutines/CoroutineScope { | ||
| public fun <init> (Laws/smithy/kotlin/runtime/io/SdkByteReadChannel;Lkotlin/coroutines/CoroutineContext;)V | ||
| public fun getCoroutineContext ()Lkotlin/coroutines/CoroutineContext; | ||
| public fun resetPosition ()Z | ||
| public fun sendRequestBody (Laws/sdk/kotlin/crt/io/MutableBuffer;)Z | ||
| } | ||
|
|
||
| public final class aws/smithy/kotlin/runtime/crt/SdkDefaultIO { | ||
| public static final field INSTANCE Laws/smithy/kotlin/runtime/crt/SdkDefaultIO; | ||
| public final fun getClientBootstrap ()Laws/sdk/kotlin/crt/io/ClientBootstrap; | ||
| public final fun getEventLoop ()Laws/sdk/kotlin/crt/io/EventLoopGroup; | ||
| public final fun getHostResolver ()Laws/sdk/kotlin/crt/io/HostResolver; | ||
| public final fun getTlsContext ()Laws/sdk/kotlin/crt/io/TlsContext; | ||
| } | ||
|
|
||
| public final class aws/smithy/kotlin/runtime/crt/SdkSourceBodyStream : aws/sdk/kotlin/crt/http/HttpRequestBodyStream { | ||
| public fun <init> (Laws/smithy/kotlin/runtime/io/SdkSource;)V | ||
| public fun resetPosition ()Z | ||
| public fun sendRequestBody (Laws/sdk/kotlin/crt/io/MutableBuffer;)Z | ||
| } | ||
|
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No actually, I think the current
typescover 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 thechangelog-verification.ymljob) and was curiousThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: