Skip to content

Add check-for-breaking-api-changes.sh script #21

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

Merged

Conversation

simonjbeaumont
Copy link
Collaborator

Motivation

We want to know when we are making breaking changes to our public API.

Modifications

Add a script that wraps swift package diagnose-api-breaking-changes which we'll start to use in CI.

Result

A script can be used to validate the current repo state against a baseline repo and treeish for breaking API changes.

Notes

Running this script produces the following output:

BASELINE_REPO_URL=https://github.com/apple/swift-openapi-runtime BASELINE_TREEISH=main bash scripts/check-for-breaking-api-changes.sh
** Checking required environment variables...
** Fetching baseline: https://github.com/apple/swift-openapi-runtime#main...
From https://github.com/apple/swift-openapi-runtime
 * branch            main       -> FETCH_HEAD
** Checking for API changes since https://github.com/apple/swift-openapi-runtime#main (e81f70f2a8b445224dc6b97d0c31ed0d190e04de)...
Building for debugging...

Build complete! (7.53s)

No breaking changes detected in OpenAPIRuntime
** ✅ No breaking API changes detected.

Then making the following local change...

diff --git a/Sources/OpenAPIRuntime/Conversion/Converter.swift b/Sources/OpenAPIRuntime/Conversion/Converter.swift
index 2aac666..f2abb63 100644
--- a/Sources/OpenAPIRuntime/Conversion/Converter.swift
+++ b/Sources/OpenAPIRuntime/Conversion/Converter.swift
@@ -35,7 +35,8 @@ public struct Converter: Sendable {

     /// Creates a new converter with the behavior specified by the configuration.
     public init(
-        configuration: Configuration
+        configuration: Configuration,
+        i: Int = 1
     ) {
         self.configuration = configuration

...and rerunning the script, gives the following output...

...
1 breaking change detected in OpenAPIRuntime:
  💔 API breakage: constructor Converter.init(configuration:) has been removed
** ERROR: ❌ Breaking API changes detected.

@simonjbeaumont simonjbeaumont marked this pull request as ready for review June 26, 2023 10:55
@simonjbeaumont simonjbeaumont requested a review from czechboy0 June 26, 2023 21:20
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Looks good! Is it intentional that this PR adds the script, but doesn't call it from docker-compose? Will that come in a subsequent PR or was it meant to be done here as well?

@simonjbeaumont
Copy link
Collaborator Author

Looks good! Is it intentional that this PR adds the script, but doesn't call it from docker-compose? Will that come in a subsequent PR or was it meant to be done here as well?

It looks to me like other projects don't have an explicit Compose service for this action. Instead the pipeline calls docker-compose run shell. Here's an example execution from a recent PR on swift-nio:

 docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.2004.56.yaml -p swift-nio2-api-breakage-prb run --rm shell -cl 'scripts/check_no_api_breakages.sh https://github.com/apple/swift-nio 60e4f7ba2b0382dea61ebe87af87b129ea631a5a main'

We'll need something similar in CI—just that we'll environment variables so that we use named arguments.

Based on this I didn't add an explicit Compose service.

@simonjbeaumont
Copy link
Collaborator Author

@yim-lee please could you set up a pipeline that runs the following command?

docker-compose -f docker/docker-compose.yaml run \
  -e BASELINE_REPO_URL=https://github.com/apple/swift-openapi-runtime \
  -e BASELINE_TREEISH=main \
  shell scripts/check-for-breaking-api-changes.sh

We can use this PR as a canary.

//cc @czechboy0

@yim-lee
Copy link
Member

yim-lee commented Jun 27, 2023

@swift-server-bot test this please

@yim-lee
Copy link
Member

yim-lee commented Jun 27, 2023

@simonjbeaumont
Copy link
Collaborator Author

Great, thanks @yim-lee!

@czechboy0 I'm going to add a commit to this PR to test it shows a break, then I'll revert and we can land this one.

@simonjbeaumont
Copy link
Collaborator Author

Great, with the canary commit above, the pipeline fails as we hoped:

1 breaking change detected in OpenAPIRuntime:
  💔 API breakage: constructor Converter.init(configuration:) has been removed
** ERROR: ❌ Breaking API changes detected.
1
Build step 'Execute shell' marked build as failure

— source: https://ci.swiftserver.group/job/swift-openapi-runtime-api-breakage-prb/2/console

@simonjbeaumont
Copy link
Collaborator Author

//cc @czechboy0 think we're good to merge this now (see above to see it in action).

@czechboy0
Copy link
Contributor

Great! Ship it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants