chore: refactor internal exprparser wrappers to reduce complexity, add new test cases #1566

Closed

mfenniak wants to merge 3 commits from mfenniak/forgejo-runner:refactor-wrappers into main

pull from: mfenniak/forgejo-runner:refactor-wrappers

merge into: forgejo:main

forgejo:main

forgejo:renovate/golang.org-x-sys-0.x

forgejo:renovate/github.com-powerman-fileuri-0.x

forgejo:renovate/forgejo-lxc-13.x

forgejo:validate-1460

forgejo:chore/testing-trixie

forgejo:chore/testing-bookworm

forgejo:expand-reusable-workflows-working-branch

forgejo:feat-default_actions_url-on-step

forgejo:2023-05-22-main-before-force-push

forgejo:2023-04-30-main-before-force-push

Conversation 2 Commits 3 Files changed 6 +219 -90

mfenniak commented 2026-06-12 00:41:55 +00:00

Owner

Copy link

Copy Markdown

#1563 introduced some new internal types in the exprparser library to allow detecting access to env.* and secrets.*. I realized that the fixes in that PR to allow toJSON(env) to work were a bit hacky, and so this PR replaces those internal wrapper types with a map[string]X where X is specific to each wrapped context, allowing toJSON(...) to work without specific detection of the wrappers. This was applied to the older matrix wrapper as well.

An additional minor issue where env.* and secrets.* (not likely to be used in an if block) weren't detected as accesses to those contexts is also fixed, and unit tests are added to cover that case.

• other
  • PR: chore: refactor internal exprparser wrappers to reduce complexity, add new test cases

#1563 introduced some new internal types in the `exprparser` library to allow detecting access to `env.*` and `secrets.*`. I realized that the fixes in that PR to allow `toJSON(env)` to work were a bit hacky, and so this PR replaces those internal wrapper types with a `map[string]X` where `X` is specific to each wrapped context, allowing `toJSON(...)` to work without specific detection of the wrappers. This was applied to the older matrix wrapper as well. An additional minor issue where `env.*` and `secrets.*` (not likely to be used in an `if` block) weren't detected as accesses to those contexts is also fixed, and unit tests are added to cover that case. <!--start release-notes-assistant--> <!--URL:https://code.forgejo.org/forgejo/runner--> - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/1566): <!--number 1566 --><!--line 0 --><!--description Y2hvcmU6IHJlZmFjdG9yIGludGVybmFsIGV4cHJwYXJzZXIgd3JhcHBlcnMgdG8gcmVkdWNlIGNvbXBsZXhpdHksIGFkZCBuZXcgdGVzdCBjYXNlcw==-->chore: refactor internal exprparser wrappers to reduce complexity, add new test cases<!--description--> <!--end release-notes-assistant-->

mfenniak added 1 commit 2026-06-12 00:41:55 +00:00

chore: fix typo in name interpreter ef190bb45c

mfenniak force-pushed refactor-wrappers from 404e9bb844 to 288f3eac49 2026-06-12 00:48:52 +00:00 Compare

aahlenst commented 2026-06-12 13:28:24 +00:00

Member

Copy link

Copy Markdown

All three integration tests agree that TestRunner_RunEvent/job-container-image has problems. That means it could be a real test failure.

All three integration tests agree that `TestRunner_RunEvent/job-container-image` has problems. That means it could be a real test failure.

mfenniak commented 2026-06-12 15:28:08 +00:00

Author

Owner

Copy link

Copy Markdown

Yikes, that was a mess to debug. This refactor won't work for a subtle reason that tests barely caught -- when running a job, the runner creates an evaluation context that references the env map in-memory, then evaluates the contents of the env map, and then uses that context for runs-on. The problem with this change is that data is copied into the evaluation context to change its type, so the middle evaluation isn't reflected.

I'm going to close this PR, rescue the other tests and bugfixes, and leave the refactoring idea dead.

Yikes, that was a mess to debug. This refactor won't work for a subtle reason that tests barely caught -- when running a job, the runner creates an evaluation context that references the `env` map in-memory, then evaluates the contents of the env map, and then uses that context for `runs-on`. The problem with this change is that data is *copied* into the evaluation context to change its type, so the middle evaluation isn't reflected. I'm going to close this PR, rescue the other tests and bugfixes, and leave the refactoring idea dead.

mfenniak closed this pull request 2026-06-12 15:28:08 +00:00

mfenniak referenced this pull request 2026-06-12 15:31:15 +00:00

fix: expression evaluation fixes discovered while refactoring 'if' detection #1572

mfenniak referenced this pull request from a commit 2026-06-12 20:11:02 +00:00

fix: expression evaluation fixes discovered while refactoring 'if' detection (#1572)

Some checks failed

Hide checks

issue-labels / release-notes (pull_request_target) Successful in 9s

Details

checks / Build Forgejo Runner (pull_request) Successful in 44s

Required

Details

checks / validate mocks (pull_request) Successful in 39s

Details

checks / validate pre-commit-hooks file (pull_request) Successful in 50s

Details

checks / Build unsupported platforms (pull_request) Successful in 55s

Details

checks / runner exec tests (pull_request) Successful in 2m5s

Required

Details

checks / Run integration tests with Docker (docker-latest) (pull_request) Failing after 9m39s

Required

Details

checks / Run integration tests with Podman (pull_request) Failing after 11m31s

Required

Details

checks / Run integration tests with Docker (docker-stable) (pull_request) Failing after 12m1s

Required

Details

test-multi-platform / validate pre-commit-hooks file (pull_request) Has been skipped

Details

test-multi-platform / validate mocks (pull_request) Has been skipped

Details

cascade / debug (pull_request_target) Has been skipped

Required

Details

test-multi-platform / Build Forgejo Runner (pull_request) Has been skipped

Details

test-multi-platform / Build unsupported platforms (pull_request) Has been skipped

Details

cascade / end-to-end (pull_request_target) Successful in 6s

Required

Details

test-multi-platform / Run integration tests with Docker (docker-latest) (pull_request) Has been skipped

Details

test-multi-platform / runner exec tests (pull_request) Has been skipped

Details

test-multi-platform / Run integration tests with Docker (docker-stable) (pull_request) Has been skipped

Details

test-multi-platform / Run integration tests with Podman (pull_request) Has been skipped

Details

test-multi-platform / arm64 (pull_request) Has been skipped

Details

cascade / forgejo (pull_request_target) Successful in 1m38s

Required

Details

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

Reviewers

No reviewers

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

2 participants

Notifications

Subscribe

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!1566

No description provided.