Skip to content

chore(ci): sanitize branch names for connect popup build#18700

Closed
Lemonexe wants to merge 1 commit intodevelopfrom
ci/connect-popup-build-sanitize-branch-name
Closed

chore(ci): sanitize branch names for connect popup build#18700
Lemonexe wants to merge 1 commit intodevelopfrom
ci/connect-popup-build-sanitize-branch-name

Conversation

@Lemonexe
Copy link
Copy Markdown
Contributor

@Lemonexe Lemonexe commented May 2, 2025

Description

Sanitize branch names for connect popup build base URL

🔍🖥️ Suite web test results: View in Currents

🔍🖥️ Suite desktop test results: View in Currents

@Lemonexe Lemonexe added no-project This label is used to specify that PR doesn't need to be added to a project DX Enhancing developer experience connect-popup Connect popup used by 3rd parties labels May 2, 2025
@Lemonexe Lemonexe force-pushed the ci/connect-popup-build-sanitize-branch-name branch from 41fb3d2 to 3fa72c9 Compare May 2, 2025 19:29
@trezor-bot
Copy link
Copy Markdown
Contributor

trezor-bot Bot commented May 2, 2025

✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@trezor-bot
Copy link
Copy Markdown
Contributor

trezor-bot Bot commented May 2, 2025

✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@Lemonexe Lemonexe marked this pull request as ready for review May 2, 2025 19:40
@Lemonexe Lemonexe requested a review from vdovhanych as a code owner May 2, 2025 19:40
- name: Sanitize branch name for URLs
id: sanitize_branch
run: |
echo "sanitized_branch=$(echo '${{ steps.extract_branch.outputs.branch }}' | tr -d '+#@&=?\\:*')" >> "$GITHUB_OUTPUT"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

technically, chars ?\:* do not need to be handled because that's an invalid branch name (git won't allow to create it), but I put it there anyway so it's clearer what kind of bugs does this solve

Copy link
Copy Markdown
Member

@vdovhanych vdovhanych left a comment

Choose a reason for hiding this comment

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

did we hit this issue somewhere already?

@Lemonexe
Copy link
Copy Markdown
Contributor Author

Lemonexe commented May 7, 2025

did we hit this issue somewhere already?

@vdovhanych yes, in the PR of the action run that I linked above I did that and was wondering why connect won't build.

I looked at all branches at origin and seems that it is not a common mistake, so maybe I'm just weird 😆
Still, when someone does create such a branch name, the issue is difficult to debug, so this might save time for someone in future.

@Lemonexe
Copy link
Copy Markdown
Contributor Author

Lemonexe commented May 7, 2025

/rebase

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2025

@trezor-bot trezor-bot Bot force-pushed the ci/connect-popup-build-sanitize-branch-name branch from 3fa72c9 to 77e6d80 Compare May 7, 2025 05:24
@trezor-bot
Copy link
Copy Markdown
Contributor

trezor-bot Bot commented May 7, 2025

✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@trezor-bot
Copy link
Copy Markdown
Contributor

trezor-bot Bot commented May 7, 2025

✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@martykan
Copy link
Copy Markdown
Member

martykan commented May 7, 2025

In your example run, the deployment works but all of the tests fail. You also need to escape it in template-connect-popup-test-params.yml

Copy link
Copy Markdown
Member

@vdovhanych vdovhanych left a comment

Choose a reason for hiding this comment

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

Interesting. I would never even consider using those symbols for branch names, but yeah, if you think it's worth fixing, let's do it. I would just do it for everything, not just connect.

@Lemonexe
Copy link
Copy Markdown
Contributor Author

Lemonexe commented May 9, 2025

Ok @vdovhanych I am closing it.
I wanted to play a little with github actions, but

  • noone has this problem
    • me randomly creating a weird branch name once in a lifetime is not a reason to add more complexity 😄
  • there is much more work needed as many tests are still failing

@Lemonexe Lemonexe closed this May 9, 2025
@Lemonexe Lemonexe deleted the ci/connect-popup-build-sanitize-branch-name branch May 9, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

connect-popup Connect popup used by 3rd parties DX Enhancing developer experience no-project This label is used to specify that PR doesn't need to be added to a project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants