forgejo/runner
Watch
61
117
Fork
You've already forked runner
 117
Code Issues 48 Pull requests 11 Releases 100 Packages 1 Activity Actions

fix: don't prepend server URL with https:// if it's an empty string #1083

Merged
earl-warren merged 4 commits from drewcassidy/runner:empty-server-url-fix into main 2025-10-12 08:53:46 +00:00
Conversation 5 Commits 4 Files changed 2 +35 -12
drewcassidy commented 2025-10-11 07:00:00 +00:00
Contributor

An attempt to address #1074 in a way that makes everyone happy

Right now, any server URL that doesnt start with http:// or https:// automatically gets https:// prepended to it. When no instance is set while running forgejo-runner exec this results in the {FORGEJO,GITHUB}_SERVER_URL environment variables being set to simply "https://". Any action using the GitHub actions toolkit chokes on this as an invalid URL and fails the run with a nondescript error. This PR instead leaves empty strings alone, which the actions toolkit seems much happier with

Alternate fixes:

  • default the --forgejo-instance flag to a valid but unresolved url like 'http://example.com' so the actions don't crash
  • don't set the environment variables at all when the server URL is an empty string. This causes the actions toolkit to assume GitHub.com as a fallback
  • bug fixes
    • PR: fix: don't prepend server URL with https:// if it's an empty string
An attempt to address #1074 in a way that makes everyone happy Right now, any server URL that doesnt start with `http://` or `https://` automatically gets `https://` prepended to it. When no instance is set while running `forgejo-runner exec` this results in the `{FORGEJO,GITHUB}_SERVER_URL` environment variables being set to simply "https://". Any action using the GitHub actions toolkit chokes on this as an invalid URL and fails the run with a nondescript error. This PR instead leaves empty strings alone, which the actions toolkit seems much happier with Alternate fixes: * default the `--forgejo-instance` flag to a valid but unresolved url like 'http://example.com' so the actions don't crash * don't set the environment variables at all when the server URL is an empty string. This causes the actions toolkit to assume GitHub.com as a fallback <!--start release-notes-assistant--> <!--URL:https://code.forgejo.org/forgejo/runner--> - bug fixes - [PR](https://code.forgejo.org/forgejo/runner/pulls/1083): <!--number 1083 --><!--line 0 --><!--description Zml4OiBkb24ndCBwcmVwZW5kIHNlcnZlciBVUkwgd2l0aCBodHRwczovLyBpZiBpdCdzIGFuIGVtcHR5IHN0cmluZw==-->fix: don't prepend server URL with https:// if it's an empty string<!--description--> <!--end release-notes-assistant-->
Don't prepend server URL if its an empty string
All checks were successful
checks / validate pre-commit-hooks file (pull_request) Successful in 50s
Details
checks / validate mocks (pull_request) Successful in 1m9s
Details
checks / build and test (pull_request) Successful in 1m20s
Details
checks / runner exec tests (pull_request) Successful in 42s
Details
checks / runner integration tests (pull_request) Successful in 7m9s
Details
checks / integration tests (pull_request) Successful in 14m1s
Details
cascade / forgejo (pull_request_target) Has been skipped
Details
cascade / debug (pull_request_target) Has been skipped
Details
cascade / end-to-end (pull_request_target) Has been skipped
Details
issue-labels / release-notes (pull_request_target) Successful in 6s
Details
5163d401dc
 
Instead of getting the URL "https://" which causes some actions to fail, they will now get the empty string if no instance is set (such as when running `exec`). Some github.com/actions actions such as cache fail with cryptic errors with an invalid URL
mfenniak commented 2025-10-12 00:32:28 +00:00
Owner

This change looks good to me. Could you please add an automated test to the sibling file run_context_test.go which covers the expected behaviour?

This change looks good to me. Could you please add an automated test to the sibling file `run_context_test.go` which covers the expected behaviour?
👍 1
earl-warren changed title from Fix: Don't prepend server URL with https:// if its an empty string to fix: Don't prepend server URL with https:// if its an empty string 2025-10-12 06:43:33 +00:00
Add tests
Some checks failed
checks / build and test (pull_request) Failing after 58s
Details
checks / validate mocks (pull_request) Failing after 53s
Details
cascade / forgejo (pull_request_target) Has been skipped
Details
cascade / debug (pull_request_target) Has been skipped
Details
cascade / end-to-end (pull_request_target) Has been skipped
Details
checks / validate pre-commit-hooks file (pull_request) Successful in 36s
Details
checks / runner exec tests (pull_request) Has been skipped
Details
checks / runner integration tests (pull_request) Has been skipped
Details
issue-labels / release-notes (pull_request_target) Successful in 6s
Details
Integration tests for the release process / release-simulation (pull_request) Successful in 5m15s
Details
checks / integration tests (pull_request) Successful in 12m45s
Details
41f3020420
drewcassidy changed title from fix: Don't prepend server URL with https:// if its an empty string to fix: Don't prepend server URL with https:// if it's an empty string 2025-10-12 06:55:01 +00:00
drewcassidy reviewed 2025-10-12 06:56:58 +00:00
act/runner/run_context.go Outdated
@ -1362,4 +1356,0 @@
set("SERVER_URL", instance)
set("API_URL", instance+"/api/v1")
}
drewcassidy commented 2025-10-12 06:56:58 +00:00
Author
Contributor

I removed this since getGithubContext is doing the exact same transformation (twice...) and withGithubEnv is always run with rc.getGithubContext

I removed this since getGithubContext is doing the exact same transformation (twice...) and withGithubEnv is always run with rc.getGithubContext
earl-warren commented 2025-10-12 07:11:54 +00:00
Contributor

can you explain with references to code why this is redundant? I think you are correct but it would help me make sure there is not a weird situation where this is actually doing something different 🙏

can you explain with references to code why this is redundant? I think you are correct but it would help me make sure there is not a weird situation where this is actually doing something different 🙏
drewcassidy commented 2025-10-12 07:53:32 +00:00
Author
Contributor

The two usages of withGithubEnv are drewcassidy/runner@1ef1b8323b/act/runner/action.go (L256) and drewcassidy/runner@1ef1b8323b/act/runner/step.go (L268)

All implementations of step.getGithubContext ultimately call rc.getGithubContext

drewcassidy/runner@1ef1b8323b/act/runner/step_action_local.go (L96) drewcassidy/runner@1ef1b8323b/act/runner/step_action_remote.go (L195) drewcassidy/runner@1ef1b8323b/act/runner/step_docker.go (L43) drewcassidy/runner@1ef1b8323b/act/runner/step_run.go (L58)

Plus it just makes more sense for the context variables to get the same treatment as the environment variables

The two usages of `withGithubEnv` are https://code.forgejo.org/drewcassidy/runner/src/commit/1ef1b8323b2d546382387cc506f04c3e98ab246d/act/runner/action.go#L256 and https://code.forgejo.org/drewcassidy/runner/src/commit/1ef1b8323b2d546382387cc506f04c3e98ab246d/act/runner/step.go#L268 All implementations of `step.getGithubContext` ultimately call `rc.getGithubContext` https://code.forgejo.org/drewcassidy/runner/src/commit/1ef1b8323b2d546382387cc506f04c3e98ab246d/act/runner/step_action_local.go#L96 https://code.forgejo.org/drewcassidy/runner/src/commit/1ef1b8323b2d546382387cc506f04c3e98ab246d/act/runner/step_action_remote.go#L195 https://code.forgejo.org/drewcassidy/runner/src/commit/1ef1b8323b2d546382387cc506f04c3e98ab246d/act/runner/step_docker.go#L43 https://code.forgejo.org/drewcassidy/runner/src/commit/1ef1b8323b2d546382387cc506f04c3e98ab246d/act/runner/step_run.go#L58 Plus it just makes more sense for the context variables to get the same treatment as the environment variables
earl-warren commented 2025-10-12 08:52:38 +00:00
Contributor

Crystal clear. Thanks for making it easy for me to follow your footsteps 🙏

Crystal clear. Thanks for making it easy for me to follow your footsteps 🙏
earl-warren marked this conversation as resolved
Make the linter happy
All checks were successful
checks / validate mocks (pull_request) Successful in 57s
Details
checks / validate pre-commit-hooks file (pull_request) Successful in 37s
Details
checks / build and test (pull_request) Successful in 3m24s
Details
checks / runner exec tests (pull_request) Successful in 32s
Details
Integration tests for the release process / release-simulation (pull_request) Successful in 5m7s
Details
checks / runner integration tests (pull_request) Successful in 5m36s
Details
checks / integration tests (pull_request) Successful in 11m35s
Details
issue-labels / release-notes (pull_request_target) Successful in 18s
Details
cascade / debug (pull_request_target) Has been skipped
Details
cascade / end-to-end (pull_request_target) Successful in 6s
Details
cascade / forgejo (pull_request_target) Successful in 2m38s
Details
1ef1b8323b
cascading-pr commented 2025-10-12 07:10:08 +00:00
Contributor

cascading-pr updated at actions/setup-forgejo#720

cascading-pr updated at https://code.forgejo.org/actions/setup-forgejo/pulls/720
earl-warren commented 2025-10-12 07:13:36 +00:00
Contributor

@drewcassidy set the run-end-to-end-test for triggering additional tests at https://code.forgejo.org/forgejo/end-to-end/src/branch/main/actions

@drewcassidy set the `run-end-to-end-test` for triggering additional tests at https://code.forgejo.org/forgejo/end-to-end/src/branch/main/actions
earl-warren changed title from fix: Don't prepend server URL with https:// if it's an empty string to fix: don't prepend server URL with https:// if it's an empty string 2025-10-12 08:52:58 +00:00
earl-warren deleted branch empty-server-url-fix 2025-10-12 08:53:47 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
earl-warren
Labels
Clear labels   
FreeBSD

   
Kind/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Chore
It is no fun but it needs to be done, this is a chore.

  
Kind/DependencyUpdate
for pull requests that are created by renovate when updating a dependency

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

  
Windows
Windows ports is an independent project, see https://github.com/Crown0815/Forgejo-runner-windows-builder

  
linux-powerpc64le
powerpc64le

  
linux-riscv64

   
linux-s390x

   
run-end-to-end-tests
Run https://code.forgejo.org/forgejo/end-to-end with a runner compiled from this pull request

  
run-forgejo-tests
Run https://codeberg.org/forgejo/forgejo tests importing the runner from this pull request

  
run-multi-platform-tests
Label a PR for automatic execution of test suite on arm64 architecture.

No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
run-multi-platform-tests
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
forgejo/runner!1083
No description provided.