gitea/act
44
20
Fork 32
Code Issues 6 Pull Requests 6 Actions Releases 27 Activity

fix action cloning, set correct server_url for act_runner exec #68

Merged
wolfogre merged 3 commits from tomaszduda23/act:main into main 2023-06-20 07:36:12 +00:00
Conversation 0 Commits 3 Files Changed 2 +16 -7
tomaszduda23 commented 2023-06-18 22:47:22 +00:00
Contributor
Copy Link
  1. Newest act is not able to clone action based on --default-actions-url
    It might be side effect of #67.
  2. Set correct server_url, api_url, graphql_url for act_runner exec
1. Newest act is not able to clone action based on --default-actions-url It might be side effect of https://gitea.com/gitea/act/pulls/67. 2. Set correct server_url, api_url, graphql_url for act_runner exec
👍 1
tomaszduda23 added 1 commit 2023-06-18 22:47:24 +00:00
fix action cloning, set correct server_url for act_runner exeec
Some checks failed
checks / check and test (pull_request) Has been cancelled
Details
f93f6a01e4
Zettat123 approved these changes 2023-06-19 01:35:18 +00:00
Zettat123 requested changes 2023-06-19 01:37:28 +00:00
pkg/runner/run_context.go Outdated
@ -875,2 +875,4 @@
ghc.GraphQLURL = fmt.Sprintf("https://%s/api/graphql", rc.Config.GitHubInstance)
}
{ // Adapt to Gitea
Zettat123 commented 2023-06-19 01:36:58 +00:00
Member
Copy Link

This function always returns at line 844 so I think we don't need these code.

This function always returns at line 844 so I think we don't need these code.
Zettat123 commented 2023-06-19 01:47:04 +00:00
Member
Copy Link

Ah, sorry for that I didn't realize these changes is for exec, ignore the comment.

Ah, sorry for that I didn't realize these changes is for `exec`, ignore the comment.
wolfogre commented 2023-06-19 07:05:55 +00:00
Owner
Copy Link

I wonder if there's a way to avoid copying. Or we could accept this and deduplicate later.

I wonder if there's a way to avoid copying. Or we could accept this and deduplicate later.
tomaszduda23 commented 2023-06-19 15:59:03 +00:00
Author
Contributor
Copy Link

At the begging I tried to unified the code since it is already in 2 places. The problem is that it seems to be no tests that I could use to verify that things works in intended way. I'm using this for 2 weeks and functionality which I relay on was broken 3 times already. Duplication seems to be better than breaking things that people relay on.

At the begging I tried to unified the code since it is already in 2 places. The problem is that it seems to be no tests that I could use to verify that things works in intended way. I'm using this for 2 weeks and functionality which I relay on was broken 3 times already. Duplication seems to be better than breaking things that people relay on.
Zettat123 requested changes 2023-06-19 05:10:44 +00:00
pkg/runner/step_action_remote.go Outdated
@ -244,6 +240,9 @@ func (ra *remoteAction) IsCheckout() bool {
}
func (ra *remoteAction) GetAvailableCloneURL(actionURLs []string) (string, error) {
if ra.URL != "" {
Zettat123 commented 2023-06-19 05:08:34 +00:00
Member
Copy Link

I think we should not add ra.URL to actionsURLs. If ra.URL is not empty, the function should return the clone URL based on ra.URL, no detection is required since this URL is specified by the user.

And we also need to remove the line in

pkg/runner/step_action_remote.go Line 52 in f93f6a01e4
sar.remoteAction.URL = github.ServerURL
, otherwise the URL will always be rewritten to github.ServerURL even if the user has specified it.

sar.remoteAction.URL = github.ServerURL
I think we should not add `ra.URL` to `actionsURLs`. If `ra.URL` is not empty, the function should return the clone URL based on `ra.URL`, no detection is required since this URL is specified by the user. And we also need to remove the line in https://gitea.com/tomaszduda23/act/src/commit/f93f6a01e4ffc8c0e3a268871e210e229d012915/pkg/runner/step_action_remote.go#L52, otherwise the URL will always be rewritten to `github.ServerURL` even if the user has specified it. ``` sar.remoteAction.URL = github.ServerURL ```
tomaszduda23 marked this conversation as resolved
pkg/runner/step_action_remote.go Outdated
@ -257,0 +256,4 @@
// TODO
// 1. login page returns 200 so runner things that repository exists
// 2. use auth token to access private repository so no need to check login
if strings.Contains(resp.Request.URL.String(), "login") {
Zettat123 commented 2023-06-19 05:10:33 +00:00
Member
Copy Link

This is incorrect. The action's URL might contain login, like https://gitea.com/docker/login-action

This is incorrect. The action's URL might contain `login`, like https://gitea.com/docker/login-action
tomaszduda23 commented 2023-06-19 06:16:01 +00:00
Author
Contributor
Copy Link

I change it to be more explicitly. Hard to find perfect solution though.

I change it to be more explicitly. Hard to find perfect solution though.
tomaszduda23 marked this conversation as resolved
tomaszduda23 added 1 commit 2023-06-19 06:14:16 +00:00
review fixes
Some checks failed
checks / check and test (pull_request) Has been cancelled
Details
8000d78879
wolfogre reviewed 2023-06-19 07:12:52 +00:00
pkg/runner/step_action_remote.go Outdated
@ -225,15 +223,11 @@ type remoteAction struct {
Ref string
}
func (ra *remoteAction) CloneURL(defaultURL string) string {
wolfogre commented 2023-06-19 07:12:52 +00:00
Owner
Copy Link

Maybe

-func (ra *remoteAction) CloneURL(URL string) string {
+func (ra *remoteAction) CloneURL(u string) string {
Maybe ```diff -func (ra *remoteAction) CloneURL(URL string) string { +func (ra *remoteAction) CloneURL(u string) string { ```
tomaszduda23 marked this conversation as resolved
wolfogre reviewed 2023-06-19 07:18:01 +00:00
pkg/runner/step_action_remote.go Outdated
@ -257,0 +254,4 @@
// TODO
// 1. gitea login page returns 200 so runner things that repository exists
// 2. use auth token to access private repository so no need to check login
if resp.Request.Response != nil && resp.Request.Response.StatusCode == http.StatusSeeOther {
wolfogre commented 2023-06-19 07:18:01 +00:00
Owner
Copy Link

I don't think it's right. The old logic is enough.

  • What the difference between resp.Request.Response.StatusCode and resp.StatusCode?
  • If the status code isn't 200 or 404, it should fail. It's not a good idea to depend to what Gitea does with REQUIRE_SIGNIN_VIEW. You could remove it from actionURLs in config if you don't need it.
I don't think it's right. The old logic is enough. - What the difference between `resp.Request.Response.StatusCode` and `resp.StatusCode`? - If the status code isn't 200 or 404, it should fail. It's not a good idea to depend to what Gitea does with `REQUIRE_SIGNIN_VIEW`. You could remove it from `actionURLs` in config if you don't need it.
Zettat123 commented 2023-06-19 07:54:50 +00:00
Member
Copy Link

I think the bug is that the resp.StatusCode is 200 when the request is redirected to the login page. So one solution is to make the detectActionClient not follow the redirect.

var detectActionClient = http.Client{
	Timeout: 5 * time.Second,
	CheckRedirect: func(req *http.Request, via []*http.Request) error {
		return http.ErrUseLastResponse
	},
}
I think the bug is that the `resp.StatusCode` is 200 when the request is redirected to the login page. So one solution is to make the `detectActionClient` not follow the redirect. ``` var detectActionClient = http.Client{ Timeout: 5 * time.Second, CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }, } ```
tomaszduda23 commented 2023-06-19 16:06:07 +00:00
Author
Contributor
Copy Link

I think the bug is that the resp.StatusCode is 200 when the request is redirected to the login page. So one solution is to make the detectActionClient not follow the redirect.

git allows for redirection so it would not be perfect either.

$curl -i -L https://git/git/ba
HTTP/2 303 
date: Mon, 19 Jun 2023 16:01:11 GMT
location: /user/login

HTTP/2 200 
content-type: text/html; charset=UTF-8
date: Mon, 19 Jun 2023 16:01:11 GMT

The flow is like bellow so I don't think this is a bug.

> I think the bug is that the `resp.StatusCode` is 200 when the request is redirected to the login page. So one solution is to make the `detectActionClient` not follow the redirect. git allows for redirection so it would not be perfect either. ``` $curl -i -L https://git/git/ba HTTP/2 303 date: Mon, 19 Jun 2023 16:01:11 GMT location: /user/login HTTP/2 200 content-type: text/html; charset=UTF-8 date: Mon, 19 Jun 2023 16:01:11 GMT ``` The flow is like bellow so I don't think this is a bug.
tomaszduda23 commented 2023-06-19 16:12:23 +00:00
Author
Contributor
Copy Link

I don't think it's right. The old logic is enough.

ok. I reverted it.

> I don't think it's right. The old logic is enough. ok. I reverted it.
tomaszduda23 added 1 commit 2023-06-19 16:10:00 +00:00
review fixes
All checks were successful
checks / check and test (pull_request) Successful in 1m16s
Details
fd8df4f68f
tomaszduda23 changed title from fix action cloning, set correct server_url for act_runner exeec to fix action cloning, set correct server_url for act_runner exec 2023-06-19 20:34:59 +00:00
wolfogre approved these changes 2023-06-20 07:35:11 +00:00
wolfogre merged commit 515c2c429d into main 2023-06-20 07:36:12 +00:00
wolfogre referenced this issue from a commit 2023-06-20 08:33:46 +00:00
Upgrade act (#248)
ccureau referenced this issue from a commit 2023-06-22 22:45:00 +00:00
add --gitea-instance (#245)
ccureau referenced this issue from a commit 2023-06-22 22:45:00 +00:00
Upgrade act (#248)
Sign in to join this conversation.
Reviewers
No reviewers
Zettat123
wolfogre
Labels
Clear labels   
duplicate

This issue or pull request already exists

  
help wanted

Need some help

  
invalid

Something is wrong

  
kind
bug
Something is not working

  
kind
enhancement
New feature

  
kind
feature

  
kind
question
More information is needed

  
proposal

   
wontfix

This won't be fixed

No Label
duplicate
help wanted
invalid
kind
bug
kind
enhancement
kind
feature
kind
question
proposal
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 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: gitea/act#68
No description provided.
Delete Branch "tomaszduda23/act:main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?