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

ci: Use ${GITHUB_BASE_REF} as treeish for checking API break #2883

Merged

Conversation

simonjbeaumont
Copy link
Contributor

@simonjbeaumont simonjbeaumont commented Sep 13, 2024

Motivation:

This repo is providing a reusable workflow for checking API breaks but it has two shortcomings:

  1. It presumes the target branch name for the PR to be main.
  2. It presumes the remote is named origin.

(2) is always the case in CI, but isn't always locally. My remote is named upstream so cannot run this using act. (1) is worse, because it's not actually testing against the target branch of the PR—consider if someone was enabling this reusable workflow on a feature branch or other-named branch.

Fortunately, Github Actions makes the target ref of the pull request available in the GITHUB_BASE_REF environment variable1.

Modifications:

Use GITHUB_BASE_REF as the argument to swift package diagnose-api-breaking-changes.

Result:

This can now be run locally.

Footnotes

  1. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables

@simonjbeaumont simonjbeaumont force-pushed the sb/use-target-ref-in-api-break-check branch 2 times, most recently from 0b9706d to 027504f Compare September 16, 2024 12:51
@simonjbeaumont simonjbeaumont force-pushed the sb/use-target-ref-in-api-break-check branch from 027504f to de3e61e Compare September 16, 2024 13:04
@simonjbeaumont simonjbeaumont added the semver/none No version bump required. label Sep 16, 2024
@simonjbeaumont simonjbeaumont marked this pull request as ready for review September 16, 2024 13:10
Copy link
Contributor

@rnro rnro left a comment

Choose a reason for hiding this comment

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

Looks great!

@FranzBusch
Copy link
Member

If I run this locally now without passing the env it will fail right? Can we make the default ref for local act runs main?

@rnro
Copy link
Contributor

rnro commented Sep 16, 2024

If I run this locally now without passing the env it will fail right? Can we make the default ref for local act runs main?

Ah, I had assumed that act took care of setting up the environment for you. I agree that if it doesn't we should have a default.

It says "the environment variables" are set up in the README https://github.com/nektos/act?tab=readme-ov-file#how-does-it-work and GITHUB_BASE_REF is mentioned in the source so it might work

@simonjbeaumont
Copy link
Contributor Author

If I run this locally now without passing the env it will fail right? Can we make the default ref for local act runs main?

I don't think so. Did it actually fail for you? When I run it without setting it, it works fine.

Ah, I had assumed that act took care of setting up the environment for you. I agree that if it doesn't we should have a default.

No default is required.

Defaulting the ref is a bad thing IMO. By defaulting it to main you are presuming the target branch of the PR, when it isn't explicitly set. When it isn't set, I would expect the best choice for default would be the default branch of the repo. Which you can get by just using nothing:

❯ git fetch https://github.com/apple/swift-nio :one

❯ git fetch https://github.com/apple/swift-nio main:two

❯ git rev-parse one
1851ae6e7d49860ea42e8ba9c4cf0aee9633d524

❯ git rev-parse two
1851ae6e7d49860ea42e8ba9c4cf0aee9633d524

@FranzBusch FranzBusch merged commit c9781cf into apple:main Sep 16, 2024
28 of 29 checks passed
cgrindel-self-hosted-renovate bot referenced this pull request in cgrindel/rules_swift_package_manager Sep 25, 2024
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [apple/swift-nio](https://redirect.github.com/apple/swift-nio) | minor
| `2.72.0` -> `2.73.0` |

---

### Release Notes

<details>
<summary>apple/swift-nio (apple/swift-nio)</summary>

###
[`v2.73.0`](https://redirect.github.com/apple/swift-nio/releases/tag/2.73.0)

[Compare
Source](https://redirect.github.com/apple/swift-nio/compare/2.72.0...2.73.0)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### SemVer Minor

- Make `ByteBuffer`'s description more useful by
[@&#8203;supersonicbyte](https://redirect.github.com/supersonicbyte) in
[https://github.com/apple/swift-nio/pull/2864](https://redirect.github.com/apple/swift-nio/pull/2864)
- Expose `UDP_MAX_SEGMENTS` via System by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2891](https://redirect.github.com/apple/swift-nio/pull/2891)
- Add new `ChannelOption` to get the amount of buffered outbound data in
the Channel by
[@&#8203;johnnzhou](https://redirect.github.com/johnnzhou) in
[https://github.com/apple/swift-nio/pull/2849](https://redirect.github.com/apple/swift-nio/pull/2849)
- Add an `AcceptBackoffHandler` to the async server bootstraps by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2782](https://redirect.github.com/apple/swift-nio/pull/2782)

##### SemVer Patch

- Adding a nicer description for `WebSocketFrame` by
[@&#8203;supersonicbyte](https://redirect.github.com/supersonicbyte) in
[https://github.com/apple/swift-nio/pull/2862](https://redirect.github.com/apple/swift-nio/pull/2862)
- Improving `description` and adding `debugDescription` to `NIOAny` by
[@&#8203;supersonicbyte](https://redirect.github.com/supersonicbyte) in
[https://github.com/apple/swift-nio/pull/2866](https://redirect.github.com/apple/swift-nio/pull/2866)
- Make FileChunk sendable by
[@&#8203;ali-ahsan-ali](https://redirect.github.com/ali-ahsan-ali) in
[https://github.com/apple/swift-nio/pull/2871](https://redirect.github.com/apple/swift-nio/pull/2871)
- Make `ByteBuffer.debugDescription` suitable for structural display by
[@&#8203;dnadoba](https://redirect.github.com/dnadoba) in
[https://github.com/apple/swift-nio/pull/2495](https://redirect.github.com/apple/swift-nio/pull/2495)
- Add support for WASILibc by
[@&#8203;MaxDesiatov](https://redirect.github.com/MaxDesiatov) in
[https://github.com/apple/swift-nio/pull/2671](https://redirect.github.com/apple/swift-nio/pull/2671)
- `NIOSingleStepByteToMessageDecoder` reentrancy safety by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2881](https://redirect.github.com/apple/swift-nio/pull/2881)
- Adopt `NIOThrowingAsyncSequenceProducer` by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2879](https://redirect.github.com/apple/swift-nio/pull/2879)
- Clamp buffer to maximum upon large write operation by
[@&#8203;ali-ahsan-ali](https://redirect.github.com/ali-ahsan-ali) in
[https://github.com/apple/swift-nio/pull/2745](https://redirect.github.com/apple/swift-nio/pull/2745)
- Revert "Adopt `NIOThrowingAsyncSequenceProducer`
([#&#8203;2879](https://redirect.github.com/apple/swift-nio/issues/2879))"
by [@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2892](https://redirect.github.com/apple/swift-nio/pull/2892)
- Add concrete description for `EmbeddedEventLoop` by
[@&#8203;aryan-25](https://redirect.github.com/aryan-25) in
[https://github.com/apple/swift-nio/pull/2890](https://redirect.github.com/apple/swift-nio/pull/2890)
- Conditionally include linux/udp.h by
[@&#8203;rnro](https://redirect.github.com/rnro) in
[https://github.com/apple/swift-nio/pull/2894](https://redirect.github.com/apple/swift-nio/pull/2894)
- Work around a type checking error when using the Static Linux SDK by
[@&#8203;euanh](https://redirect.github.com/euanh) in
[https://github.com/apple/swift-nio/pull/2898](https://redirect.github.com/apple/swift-nio/pull/2898)

##### Other Changes

- \[CI] Run tests on push to main by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2868](https://redirect.github.com/apple/swift-nio/pull/2868)
- \[CI] License header support `.in` and `.cmake` files by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2870](https://redirect.github.com/apple/swift-nio/pull/2870)
- Include nanoseconds in assertion of timestamp for NIOFileSystem tests
by [@&#8203;gjcairo](https://redirect.github.com/gjcairo) in
[https://github.com/apple/swift-nio/pull/2869](https://redirect.github.com/apple/swift-nio/pull/2869)
- Correct the link of sswg-security at SECURITY.md by
[@&#8203;lamtrinhdev](https://redirect.github.com/lamtrinhdev) in
[https://github.com/apple/swift-nio/pull/2872](https://redirect.github.com/apple/swift-nio/pull/2872)
- Speculative fix for flakey AsyncTestingEventLoop test by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2873](https://redirect.github.com/apple/swift-nio/pull/2873)
- ci: Install shellcheck if not present in CI runner by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2882](https://redirect.github.com/apple/swift-nio/pull/2882)
- ci: Use ${GITHUB_BASE_REF} as treeish for checking API break by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2883](https://redirect.github.com/apple/swift-nio/pull/2883)
- ci: Refer to nested reusable workflows using remote variant by
[@&#8203;simonjbeaumont](https://redirect.github.com/simonjbeaumont) in
[https://github.com/apple/swift-nio/pull/2884](https://redirect.github.com/apple/swift-nio/pull/2884)
- \[CI] Fix pull request label workflow by
[@&#8203;FranzBusch](https://redirect.github.com/FranzBusch) in
[https://github.com/apple/swift-nio/pull/2885](https://redirect.github.com/apple/swift-nio/pull/2885)

#### New Contributors

- [@&#8203;ali-ahsan-ali](https://redirect.github.com/ali-ahsan-ali)
made their first contribution in
[https://github.com/apple/swift-nio/pull/2871](https://redirect.github.com/apple/swift-nio/pull/2871)
- [@&#8203;aryan-25](https://redirect.github.com/aryan-25) made their
first contribution in
[https://github.com/apple/swift-nio/pull/2890](https://redirect.github.com/apple/swift-nio/pull/2890)
- [@&#8203;johnnzhou](https://redirect.github.com/johnnzhou) made their
first contribution in
[https://github.com/apple/swift-nio/pull/2849](https://redirect.github.com/apple/swift-nio/pull/2849)
- [@&#8203;euanh](https://redirect.github.com/euanh) made their first
contribution in
[https://github.com/apple/swift-nio/pull/2898](https://redirect.github.com/apple/swift-nio/pull/2898)

**Full Changelog**:
apple/swift-nio@2.72.0...2.73.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config
help](https://redirect.github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://redirect.github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC45NC4xIiwidXBkYXRlZEluVmVyIjoiMzguOTQuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: cgrindel-self-hosted-renovate[bot] <139595543+cgrindel-self-hosted-renovate[bot]@users.noreply.github.com>
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