Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support downloading raw task logs #24451

Merged
merged 4 commits into from Jun 29, 2023
Merged

Support downloading raw task logs #24451

merged 4 commits into from Jun 29, 2023

Conversation

vitalif
Copy link
Contributor

@vitalif vitalif commented Apr 30, 2023

Hi!
This pull request adds support for downloading raw task logs for Gitea Actions, similar to Github Actions
It looks like the following:
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 30, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 30, 2023
@silverwind silverwind added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/gitea-actions related to the actions of Gitea labels Apr 30, 2023
@yp05327
Copy link
Contributor

yp05327 commented May 1, 2023

What about the permission? Anyone who can access this page can download them?

@wolfogre
Copy link
Member

wolfogre commented May 4, 2023

What about the permission? Anyone who can access this page can download them?

IMO, yes. Since one can access this page, they should be able to see the log content on the page. So why can't they download the log file?

@yp05327
Copy link
Contributor

yp05327 commented May 4, 2023

What about the permission? Anyone who can access this page can download them?

IMO, yes. Since one can access this page, they should be able to see the log content on the page. So why can't they download the log file?

Do these logs have secrets?
I’m not sure where we convert secrets into ‘*’

@wolfogre
Copy link
Member

wolfogre commented May 4, 2023

What about the permission? Anyone who can access this page can download them?

IMO, yes. Since one can access this page, they should be able to see the log content on the page. So why can't they download the log file?

Do these logs have secrets? I’m not sure where we convert secrets into ‘*’

The secrets in logs will be converted into *** by runners before the logs are sent to Gitea. Here it is: https://gitea.com/gitea/act_runner/src/commit/eef3c32eb2418dcbcd40cb13076c2887d4f6913b/internal/pkg/report/reporter.go#L415

@vitalif vitalif force-pushed the raw-task-logs branch 4 times, most recently from be9b329 to d64e0fd Compare June 5, 2023 10:05
routers/web/repo/actions/view.go Outdated Show resolved Hide resolved
routers/web/repo/actions/view.go Outdated Show resolved Hide resolved
@wolfogre
Copy link
Member

wolfogre commented Jun 6, 2023

I have no more questions on the backend side, it works well on my environment. But I think there's still some room for improvement on the UI.

  • It doen't look well:
image
  • If it's unavailable to download, I can still click the button and get a text error page.
    • Maybe the button should be disabled, and show a tooltip to explain why.
image image

Not blocking, it doesn't really hurt. I'm OK to merge this first and improve it in another PR.

defer reader.Close()

ctx.ServeContent(reader, &context_module.ServeHeaderOptions{
Filename: job.Name + ".txt",
Copy link
Member

Choose a reason for hiding this comment

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

.log? Maybe the name should include the number.

Copy link
Member

@silverwind silverwind Jun 6, 2023

Choose a reason for hiding this comment

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

I would do a pattern like $workflow-$job-$id.log.

@silverwind
Copy link
Member

silverwind commented Jun 6, 2023

This alignment issue can be blamed on Fomantic UI. We could circumvent it by adding icons to all menu items. In case of "unchecked" state, we could create a gitea-empty.svg which is a SVG without any content, so it takes up space.

@silverwind
Copy link
Member

silverwind commented Jun 6, 2023

This alignment issue can be blamed on Fomantic UI. We could circumvent it by adding icons to all menu items. In case of "unchecked" state, we could create a gitea-empty.svg which is a SVG without any content, so it takes up space.

Fixed in 228bc9b:

Screenshot 2023-06-06 at 20 11 37 Screenshot 2023-06-06 at 20 16 59

@silverwind
Copy link
Member

Done with my fixes, leaving the rest to you @vitalif.

Co-authored-by: Jason Song <i@wolfogre.com>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 28, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 29, 2023
@wolfogre wolfogre added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 29, 2023
@lunny lunny added this to the 1.21.0 milestone Jun 29, 2023
@lunny lunny merged commit f0b773e into go-gitea:main Jun 29, 2023
23 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 29, 2023
@vitalif
Copy link
Contributor Author

vitalif commented Jun 29, 2023

Oh, thanks for merging, I was thinking about fixing UI :)

@silverwind
Copy link
Member

silverwind commented Jun 29, 2023

Still fixes outstanding? If so, needs a new PR :)

@vitalif
Copy link
Contributor Author

vitalif commented Jun 29, 2023

Ok, no problem :)

@yp05327
Copy link
Contributor

yp05327 commented Jul 31, 2023

@silverwind
Copy link
Member

I don't understand. Do we need to re-apply it in a separate PR?

@yp05327
Copy link
Contributor

yp05327 commented Aug 1, 2023

I don't understand. Do we need to re-apply it in a separate PR?

@silverwind
The UI now is as the following and it looks strange:
image
Then I notice that you have improved it in 228bc9b
And it looks better:
image

The reason why we lost your patch is that after you push your code, it seems that @vitalif didn't notice that and made a force push to the branch:
image
And in this force push, your patch is recovered. So I think we should re-apply it in a separate PR.

@silverwind
Copy link
Member

silverwind commented Aug 1, 2023

Indeed, the force-push eliminated my changes. Now this is rather hard to recover as all server-side data was destroyed. Maybe I still have it in my local reflog, need to check.

Maybe we should consider disabling force push for PR branches, if GitHub has such an option. I personally always force-push with --force-with-leases which ensures commits are never overwritten.

Edit: Luckily I linked the commit above, 90% sure it was only that single one.

@silverwind silverwind mentioned this pull request Aug 1, 2023
@silverwind
Copy link
Member

#26278

silverwind added a commit that referenced this pull request Aug 12, 2023
Ressurect lost changes from
#24451.

- Always show icons for each entry in the menu
- Make all checkboxes toggle only their feature, e.g. "seconds" and
"timestamps" can now be toggled on together.
- Reorder the items

<img width="845" alt="Screenshot 2023-08-01 at 19 19 27"
src="https://github.com/go-gitea/gitea/assets/115237/8a76e9bf-7966-42a6-87c9-e88cdddaec82">

---------

Co-authored-by: Giteabot <teabot@gitea.io>
vitalif added a commit to vitalif/gitea that referenced this pull request Aug 29, 2023
Hi!
This pull request adds support for downloading raw task logs for Gitea
Actions, similar to Github Actions
It looks like the following:

![image](https://user-images.githubusercontent.com/945339/235376746-405d5019-710b-468b-8113-9e82eab8e752.png)
vitalif pushed a commit to vitalif/gitea that referenced this pull request Aug 29, 2023
Ressurect lost changes from
go-gitea#24451.

- Always show icons for each entry in the menu
- Make all checkboxes toggle only their feature, e.g. "seconds" and
"timestamps" can now be toggled on together.
- Reorder the items

<img width="845" alt="Screenshot 2023-08-01 at 19 19 27"
src="https://github.com/go-gitea/gitea/assets/115237/8a76e9bf-7966-42a6-87c9-e88cdddaec82">

---------

Co-authored-by: Giteabot <teabot@gitea.io>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/gitea-actions related to the actions of Gitea type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants