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

GHA check sdk-go compatibility #200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rodrigozhou
Copy link
Contributor

What changed?
GHA check sdk-go compatibility

Why?
Automation: be able to check if a api-go version breaks sdk-go

How did you test it?

Potential risks

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Arguably this should have triggered a workflow in the Go SDK repo but this is fine.

runs-on: ubuntu-latest

steps:
- name: Validate inputs
Copy link
Member

Choose a reason for hiding this comment

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

Probably redundant since the checkout step will fail if the ref isn't valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to at least translate the "latest" input to an actual tag. The checkout step fails with the "latest" ref.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, yeah, I had it in my head that we want to verify master and not latest, but I think latest is right.
We need to give guidance though that sometimes latest is expected to fail, e.g. when making backwards incompatible changes, and that the action should be rerun with master. If that also fails, we should not cut a release.

API_REF: ${{ github.event.inputs.api_ref }}
run: |
if [[ "$SDK_REF" == "latest" ]]; then
SDK_REF=$(gh api /repos/temporalio/sdk-go/releases/latest --jq '.name')
Copy link
Member

Choose a reason for hiding this comment

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

I see you're using the GH_TOKEN env var so probably not much of a concern but this command may get rate limited by GH.

I vote for removing this step to have one less moving part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

env:
API_REF: ${{ steps.inputs.outputs.API_REF }}
run: |
for f in $(find . -iname go.mod); do
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if any of the commands in this loop fail, will this shell script error out or will these failures be silent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will error out.

@bergundy
Copy link
Member

This should run before a release is tagged and it's too late.

@cretz
Copy link
Member

cretz commented Dec 11, 2024

Arguably this should have triggered a workflow in the Go SDK repo but this is fine.

I requested it be put in here so it can be updated by anyone using this repo that may need to change instead of just something owned by the SDK team, and it's a condition on this repo not the SDK one. Granted we plan on making sure the Go SDK updates to latest API release on each release it makes as an inverse condition.

@rodrigozhou rodrigozhou requested a review from bergundy December 11, 2024 22:29
Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

IMHO the overall process should be:

  • Validate Go SDK
  • Cut a draft api release
  • Cut a draft api-go release
  • Send slack notification for approval (can come later)
  • Reviewer approves the releases (can say this needs both SDK and server)
  • Release api
  • Release api-go

@cretz
Copy link
Member

cretz commented Dec 13, 2024

Works for me. I think the first 3 can be part of the same workflow or workflow triggers we have here and in other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants