[CNSL-1930] Add automated OpenAPI spec sync workflow#108
[CNSL-1930] Add automated OpenAPI spec sync workflow#108linhcrl wants to merge 1 commit intocockroachdb:mainfrom
Conversation
b332d74 to
a8dce22
Compare
fantapop
left a comment
There was a problem hiding this comment.
I left some feedback items here but as I was getting too the bottom, it felt like a lot of logic to be putting in a local workflow file. Since this is all happening in this repo and we need to check out the repo anyways could this all live in a single build script? For example take a look at the release script in ccloud cli: https://github.com/cockroachlabs/ccloud-private/blob/main/build/release.sh
7c4c776 to
e89296d
Compare
6fc7166 to
ee7abbc
Compare
|
|
||
| - name: Run OpenAPI sync workflow | ||
| run: | | ||
| chmod +x scripts/openapi-sync.sh |
There was a problem hiding this comment.
I think we should just add +x to the actual file in the repo so it's checked out with execute
| # MANAGED_SERVICE_TOKEN: GitHub token with managed-service read access | ||
| # FORK_PUSH_TOKEN: GitHub token with fork push access | ||
| # FORK_OWNER: GitHub username that owns the fork | ||
| # GITHUB_TOKEN: GitHub token for creating PRs |
There was a problem hiding this comment.
naming seems inconsistent here. How about CREATE_PR_TOKEN?
| fi | ||
|
|
||
| export GH_TOKEN="$MANAGED_SERVICE_TOKEN" | ||
| gh api "repos/$MS_OWNER/$MS_REPO/contents/$SPEC_PATH_IN_MS?ref=$REF" \ |
There was a problem hiding this comment.
We should probably add a check at the top of the file to ensure gh is in the PATH. I know the actions will have this by default but in case someone tries to run it locally I think it would be good to have it.
There was a problem hiding this comment.
updated to check using a helper I added called check_required_commands
| sudo make generate-openapi-client | ||
| sudo chown --recursive "$(id --user):$(id --group)" internal/openapi-generator/ pkg/client/ docs/ README.md |
There was a problem hiding this comment.
I'm surprised we need sudo here. Is there a way to get around that?
There was a problem hiding this comment.
The sudo is needed because when GitHub Actions runs this script, it operates as a non-root user. The docker compose commands in the Makefile run containers as root by default, which creates the generated files (in pkg/client/, docs/, etc.) owned by root. The GitHub Actions user then cannot access or commit these files. We use sudo to run the make command, and then sudo chown to change ownership back to the GitHub Actions user so it can stage and commit the changes.
An alternative approach would be to configure the Docker Compose services to run as the current user by setting the user field in docker-compose.yml for both the jq and openapi-generator services. The script would then need to export UID and GID environment variables before calling make generate-openapi-client, and we could remove both sudo calls.
There was a problem hiding this comment.
okay got it. Let's not worry about it. Unless you've already worked on it and have it figured out..
| if [[ "$EVENT_TYPE" == "openapi-spec-merged" ]]; then | ||
| log_info "No changes detected, but adding SHA trailer to existing commit" | ||
|
|
||
| CURRENT_COMMIT_MSG=$(git log -1 --format=%B) | ||
|
|
||
| if echo "$CURRENT_COMMIT_MSG" | grep --quiet "Managed-service-commit-SHA: $MANAGED_SERVICE_SHA"; then | ||
| log_info "SHA trailer already exists in commit" | ||
| else | ||
| NEW_COMMIT_MSG=$(printf "%s\nManaged-service-commit-SHA: %s" "$CURRENT_COMMIT_MSG" "$MANAGED_SERVICE_SHA") | ||
| git commit --amend --message "$NEW_COMMIT_MSG" | ||
| fi |
There was a problem hiding this comment.
can you tell me more about how this happens in the workflow? I can't quite tell what's going on here. If a commit has already landed in main we shouldn't try to amend it.
There was a problem hiding this comment.
I added a comment above the code block to explain what's happening here. Let me know if that helps
| MANAGED_SERVICE_SHA="${2:-}" | ||
|
|
||
| if [[ -z "${MANAGED_SERVICE_PR_URL:-}" ]]; then | ||
| echo "::error::managed_service_pr_url is required" >&2 |
There was a problem hiding this comment.
can these use the logging helpers?
| # Output an informational message to stderr | ||
| log_info() { | ||
| local message="$1" | ||
| echo "$message" >&2 |
There was a problem hiding this comment.
I think I may have asked this in another PR but it seems like info and notice level logs should go to stdout if possible.
There was a problem hiding this comment.
now we are directing info and notice to stdout
| local managed_service_pr_url="$1" | ||
|
|
||
| if [[ -z "${managed_service_pr_url:-}" ]]; then | ||
| log_error "managed_service_pr_url is required" |
There was a problem hiding this comment.
do we need to include the logging.sh? Or is it assumed to included by the invoking script already? If it's assumed we should probably check that it's available when this script is invoked and fail with a message in case someone tries to use it differently.
There was a problem hiding this comment.
i decided to source in each file
| set -e | ||
|
|
||
| if [ -z "$GIT_FORK_USER" ] || [ -z "$GIT_FORK_PASSWORD" ]; then | ||
| echo "::error::GIT_FORK_USER and GIT_FORK_PASSWORD must be set" >&2 |
There was a problem hiding this comment.
should this use the logging helpers?
| # Create prompt from template with substitutions | ||
| PROMPT_FILE=$(mktemp) | ||
| sed "s|{{MANAGED_SERVICE_PR_URL}}|$MANAGED_SERVICE_PR_URL|g" "$PROMPT_TEMPLATE" > "$PROMPT_FILE" |
There was a problem hiding this comment.
Nit: templatizing with this url doesn't seem to dangerous but in generally it's a pattern that can lead to prompt injection. Another way you can do this is to make sure the url is in the env that claude is called with and tell it what the env var is in the prompt. For example
- Look up the managed service url from the environment using printenv MANAGED_SERVICE_PR_URL
- ensure.. the trailer exists and contains the text in this exact format but with the value of the env var: managed-service-url: <MANAGED_SERVICE_PR_URL>
- etc...
209781d to
d4b826d
Compare
This workflow automates synchronization of OpenAPI specs from the managed-service repository. When triggered via workflow_dispatch with `openapi-spec-changed` or `openapi-spec-merged` event types: - Fetches the latest OpenAPI spec from the managed-service PR branch (for changed events) or from the specific commit SHA (for merged events) - Regenerates SDK client code using make generate-openapi-client - Invokes Claude AI (via Vertex AI) to analyze changes and update CHANGELOG.md with appropriate entries - Creates or updates PR from bot fork to pending-deploy branch The workflow includes scripts for finding corresponding SDK PRs and invoking Claude for changelog generation. The Claude prompt instructs it to analyze git diffs, update CHANGELOG.md following Keep a Changelog conventions, identify breaking changes, and output structured metadata for commit messages and PR descriptions. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Example PR opened by workflow
openapi-spec-changedevent: https://github.com/cockroachlabs/cockroach-cloud-sdk-go-automation-testing/pull/41openapi-spec-mergedevent: https://github.com/cockroachlabs/cockroach-cloud-sdk-go-automation-testing/pull/43Note: in the case of the "merged" event, there's an additional trailer in the commit message
This workflow automates synchronization of OpenAPI specs from the managed-service repository. When managed-service dispatches
openapi-spec-changedoropenapi-spec-mergedevents:The workflow includes scripts for finding corresponding SDK PRs and invoking Claude for changelog generation. The Claude prompt instructs it to analyze git diffs, update CHANGELOG.md following Keep a Changelog conventions, identify breaking changes, and output structured metadata for commit messages and PR descriptions.
Supports manual testing via workflow_dispatch trigger.