forgejo/act
Archived
24
5
Fork 16
Code Issues Pull requests Releases Activity Actions

fix: image credentials for services must not override container image credentials #181

Merged
earl-warren merged 3 commits from earl-warren/act:wip-credentials into main 2025-07-15 20:55:38 +00:00
Conversation 4 Commits 3 Files changed 5 +342 -145
earl-warren commented 2025-07-15 13:03:31 +00:00
Contributor
Copy link
  • do not override username and password when looping over services
  • split prepareJobContainer out of startJobContainer
  • split getNetworkName out as it is used by both
  • add unit tests for prepareJobContainer
  • make containre.NewContainer mockable
  • add MockVariable helper

Closes forgejo/runner#575

Note to reviewers: do not show whitespace change, the refactor will show in a minimal way. When the fix is reverted the tests fail as follows:

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -81,4 +81,4 @@
        	            	   Image: (string) (len=10) "some:image",
        	            	-  Username: (string) (len=17) "containerusername",
        	            	-  Password: (string) (len=17) "containerpassword",
        	            	+  Username: (string) (len=16) "service2username",
        	            	+  Password: (string) (len=16) "service2password",
        	            	   Entrypoint: ([]string) (len=3) {
        	Test:       	TestStartJobContainer/Overlapping
- do not override username and password when looping over services - split prepareJobContainer out of startJobContainer - split getNetworkName out as it is used by both - add unit tests for prepareJobContainer - make containre.NewContainer mockable - add MockVariable helper Closes forgejo/runner#575 --- Note to reviewers: do not show whitespace change, the refactor will show in a minimal way. When the fix is reverted the tests fail as follows: ``` Diff: --- Expected +++ Actual @@ -81,4 +81,4 @@ Image: (string) (len=10) "some:image", - Username: (string) (len=17) "containerusername", - Password: (string) (len=17) "containerpassword", + Username: (string) (len=16) "service2username", + Password: (string) (len=16) "service2password", Entrypoint: ([]string) (len=3) { Test: TestStartJobContainer/Overlapping ```
fix: image credentials for services must not override container image credentials
All checks were successful
checks / unit (pull_request) Successful in 1m57s
Details
checks / integration (pull_request) Successful in 1m9s
Details
/ cascade (pull_request_target) Successful in 33m30s
Details
544c7e58f8
 
Closes forgejo/runner#575
earl-warren commented 2025-07-15 13:04:25 +00:00
Author
Contributor
Copy link

Now to the hard part... testing.

Now to the hard part... testing.
cascading-pr commented 2025-07-15 13:04:28 +00:00
Contributor
Copy link

cascading-pr updated at forgejo/runner#700

cascading-pr updated at https://code.forgejo.org/forgejo/runner/pulls/700
earl-warren changed title from WIP: fix: image credentials for services must not override container image credentials to WIP: fix: image credentials for services must not override container image credentials [skip cascade] 2025-07-15 18:21:42 +00:00
earl-warren force-pushed wip-credentials from 352508b798
Some checks failed
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Failing after 37s
Details
checks / integration (pull_request) Has been skipped
Details
to a82bc9ebbe
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m17s
Details
checks / integration (pull_request) Successful in 1m8s
Details
2025-07-15 18:30:22 +00:00 Compare
earl-warren commented 2025-07-15 18:36:53 +00:00
Author
Contributor
Copy link

It was difficult in the sense that there was nothing to get inspiration from. But in the end it is not so bad. Continuations and closures inspired from JavaScript could have made it almost impossible. But the state passed along in the chain of Executor is stored in structs that can be examined by tests. Some would argue this is not how functional programming should work but that serves the purpose of adding tests after the fact so there is no reason to complain.

It was difficult in the sense that there was nothing to get inspiration from. But in the end it is not so bad. Continuations and closures inspired from JavaScript could have made it almost impossible. But the state passed along in the chain of Executor is stored in structs that can be examined by tests. Some would argue this is not how functional programming should work but that serves the purpose of adding tests after the fact so there is no reason to complain.
earl-warren changed title from WIP: fix: image credentials for services must not override container image credentials [skip cascade] to fix: image credentials for services must not override container image credentials [skip cascade] 2025-07-15 18:37:23 +00:00
earl-warren changed title from fix: image credentials for services must not override container image credentials [skip cascade] to fix: image credentials for services must not override container image credentials 2025-07-15 18:37:46 +00:00
earl-warren changed title from fix: image credentials for services must not override container image credentials to fix: image credentials for services must not override container image credentials [skip cascade] 2025-07-15 18:38:41 +00:00
earl-warren force-pushed wip-credentials from a82bc9ebbe
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m17s
Details
checks / integration (pull_request) Successful in 1m8s
Details
to 4a56a5247b
Some checks failed
checks / unit (pull_request) Successful in 1m19s
Details
checks / integration (pull_request) Successful in 1m30s
Details
/ cascade (pull_request_target) Failing after 34s
Details
2025-07-15 18:39:48 +00:00 Compare
viceice approved these changes 2025-07-15 18:50:36 +00:00
viceice left a comment
Owner

LGTM

LGTM
earl-warren changed title from fix: image credentials for services must not override container image credentials [skip cascade] to fix: image credentials for services must not override container image credentials 2025-07-15 20:24:33 +00:00
earl-warren deleted branch wip-credentials 2025-07-15 20:55:38 +00:00
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Kwonunn
viceice
Labels
Clear labels   
Compat/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind
Chore
Housekeeping, cleanup and chores

  
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

No labels
Compat/Breaking
Kind/Bug
Kind
Chore
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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
earl-warren
3 participants
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/act!181
No description provided.