Skip to content

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Sep 8, 2025

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@fisx fisx requested a review from a team as a code owner September 8, 2025 14:28
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 8, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

Something is strange here: In line 25 we cd wire-docs.

So, I'm under the impression that this job is about building the docs. Sanitizing the PR would in my head be a dedicated job - if we want to load stuff onto Github instead of Concourse (which I would endorse for simple stuff like this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh, you're right? thinking fast doesn't always mean thinking good... sorry, i'll try again!

Copy link
Contributor

@supersven supersven left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of our developer experience! However, this one might need a bit more thought 😸

@fisx fisx force-pushed the add-sanitize-pr-to-github-workflow branch from b438b8b to 8cf8a47 Compare September 12, 2025 14:38
@fisx fisx requested a review from blackheaven September 12, 2025 14:39
@fisx fisx requested a review from Copilot October 6, 2025 09:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds execution of make sanitize-pr within the CI workflow to automatically apply PR sanitation rules.

  • Introduces a new job step that clones the repository and runs make sanitize-pr
  • Adds shell logic to derive the branch name from GITHUB_REF

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

CURRENT_REPO_URL="https://github.com/${GITHUB_REPOSITORY}.git"
BRANCH_NAME=${GITHUB_REF##*/}
git clone --branch develop "$CURRENT_REPO_URL" wire-server -b "$BRANCH_NAME"
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

The git clone command passes a second -b/--branch option after the destination directory, which git interprets as an extra argument and will fail (e.g. "fatal: repository '-b' does not exist"). Use a single --branch before the repo URL, or implement an explicit fallback. Example: git clone --branch "$BRANCH_NAME" "$CURRENT_REPO_URL" wire-server || git clone --branch develop "$CURRENT_REPO_URL" wire-server (with cleanup as needed).

Suggested change
git clone --branch develop "$CURRENT_REPO_URL" wire-server -b "$BRANCH_NAME"
# Try to clone with the PR branch, fall back to develop if it doesn't exist
if ! git clone --branch "$BRANCH_NAME" "$CURRENT_REPO_URL" wire-server; then
rm -rf wire-server
git clone --branch develop "$CURRENT_REPO_URL" wire-server
fi

Copilot uses AI. Check for mistakes.

# if this fails, you need to run it on your PR and commit the changes it makes.
run: |
CURRENT_REPO_URL="https://github.com/${GITHUB_REPOSITORY}.git"
BRANCH_NAME=${GITHUB_REF##*/}
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

Using ${GITHUB_REF##*/} will produce 'merge' for pull_request refs like refs/pull/123/merge, causing the subsequent clone to target a non-existent branch. Prefer GITHUB_HEAD_REF when the event is pull_request, falling back to GITHUB_REF for push events, e.g.: BRANCH_NAME="${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}".

Suggested change
BRANCH_NAME=${GITHUB_REF##*/}
BRANCH_NAME="${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}"

Copilot uses AI. Check for mistakes.

Comment on lines +41 to +50
- name: Clone wire-server and `make sanitize-pr`
# if this fails, you need to run it on your PR and commit the changes it makes.
run: |
CURRENT_REPO_URL="https://github.com/${GITHUB_REPOSITORY}.git"
BRANCH_NAME=${GITHUB_REF##*/}
git clone --branch develop "$CURRENT_REPO_URL" wire-server -b "$BRANCH_NAME"
cd wire-server
make sanitize-pr
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

This step reclones the same repository that has already been checked out earlier in the job, adding unnecessary network and I/O cost. You can run make sanitize-pr directly in the existing workspace (remove clone/cd) unless isolation is required; if isolation is needed, add a shallow clone (e.g. --depth 1) and fix the branch logic as noted.

Suggested change
- name: Clone wire-server and `make sanitize-pr`
# if this fails, you need to run it on your PR and commit the changes it makes.
run: |
CURRENT_REPO_URL="https://github.com/${GITHUB_REPOSITORY}.git"
BRANCH_NAME=${GITHUB_REF##*/}
git clone --branch develop "$CURRENT_REPO_URL" wire-server -b "$BRANCH_NAME"
cd wire-server
make sanitize-pr
- name: Run `make sanitize-pr`
# if this fails, you need to run it on your PR and commit the changes it makes.
run: |
make sanitize-pr

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants