Link to executed workflow #31906
Link to executed workflow #31906
Conversation
GiteaBot
added
the
lgtm/need 2
github-actions
bot
added
modifies/translation
modifies/go
There was a problem hiding this comment.
Nice, very helpful to have the link to the workflow file.
You've created a generic function to return relative path for a tree object, which we can use in other instances. I think this is a good solution.
I guess the alternative (for this particular case) would be to save the entire relative path to the workflow file in the actions table. I think that's more complex than needed, and this avoids the migration at the moment.
I think this could use either unit test or integration test. Otherwise LGTM!
GiteaBot
added
lgtm/blocked
| @@ -179,3 +181,30 @@ func (tes Entries) Sort() { | |||
| func (tes Entries) CustomSort(cmp func(s1, s2 string) bool) { | |||
| sort.Sort(customSortableEntries{cmp, tes}) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Adding a note for other reviewers: this file doesn't differentiate between go-git and non-go-git impl, so it won't need multiple implementations based on which backend is used.
There was a problem hiding this comment.
Total side-tangent, but does anyone use go-git?
There was a problem hiding this comment.
go-git version has been used in almost all Windows users.
| for current != nil && current.ptree != nil { | ||
| entries, err := current.ptree.ListEntries() | ||
| if err != nil { | ||
| log.Error("Failed to climb git tree %v", err) | ||
| return "" | ||
| } | ||
| for _, entry := range entries { | ||
| if entry.ID.String() == current.ID.String() { | ||
| path = entry.Name() + "/" + path | ||
| break | ||
| } | ||
| } | ||
| current = current.ptree | ||
| } |
There was a problem hiding this comment.
This looks like a really inefficient way to gather the full path of a file.
However, I know that this function is drastically needed, also for other functionality (see for example #28835 which got stalled exactly as I wasn't sure how to implement this)._
Thus, I'll allow it for now as long as we get it merged.
We can still improve its performance in the future with a precalculated path.
There was a problem hiding this comment.
Agreed. I think a TreeEntry should have its absolute path. But, felt out of scope for this requested feature.
There was a problem hiding this comment.
I also agree with delvh, we can't do so. #31906 (comment)
GiteaBot
added
lgtm/done
william-allspice
force-pushed
the
allspice/workflow-link
branch
from
682bf49 to
9ded4a7
Compare
| "io" | ||
| "strings" | ||
|
|
||
| actions_model "code.gitea.io/gitea/models/actions" |
There was a problem hiding this comment.
modules is the wrong place for this.
Go does not allow import cycles.
Please do not add any imports to models from modules.
There was a problem hiding this comment.
modules is the wrong place for this.
@delvh can you expand on what you are asking for?
It sounds like you are asking to move this function and the ListWorkflows function to models? In my humble opinion, I don't think this makes sense because there is no existing concept of a workflow in the database which to me is the equivalent of the models. Happy to hear a different opinion.
I could refactor a lot of the workflow logic, including ListWorkflows, to live in a new file called models/actions/workflows. But it feels odd since there is no database table for a workflow and would violate this principle:
models: Contains the data structures used by xorm to construct database tables. It also contains functions to query and update the database. Dependencies to other Gitea code should be avoided. You can make exceptions in cases such as logging.
Outlined here: https://docs.gitea.com/contributing/guidelines-backend
This module is already dependent on the actions model here: https://github.com/go-gitea/gitea/blob/main/modules/actions/task_state.go#L7
There was a problem hiding this comment.
This module is already dependent on the actions model here: https://github.com/go-gitea/gitea/blob/main/modules/actions/task_state.go#L7
🤮 Okay, that is one of the places we didn't fix yet.
We are still working on removing all the historically grown errors.
For the time being, I think it is better not to introduce any already known technical debt.
As to where to add it, I would have added it as a type method on actions_model.ActionRun which is defined in models/actions/run.go
GiteaBot
added
lgtm/blocked
| return "" | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s/src/commit/%s/%s", run.Repo.Link(), run.CommitSHA, workflowFilePath) |
There was a problem hiding this comment.
It needs proper escaping, otherwise the link might be invalid
There was a problem hiding this comment.
Agreed, we can't influence the workflow file
| @@ -179,3 +181,30 @@ func (tes Entries) Sort() { | |||
| func (tes Entries) CustomSort(cmp func(s1, s2 string) bool) { | |||
| sort.Sort(customSortableEntries{cmp, tes}) | |||
| } | |||
|
|
|||
| // GetPathInRepo returns the relative path in the tree to this entry | |||
| func (te *TreeEntry) GetPathInRepo() string { | |||
There was a problem hiding this comment.
This file was organized by this:
TreeEntry code
Entries code
But now it becomes:
TreeEntry code
Entries code
TreeEntry code
The newly added code should be in the existing "TreeEntry code" section.
|
Stale? |
wxiaoguang
added
the
issue/needs-feedback
| return "" | ||
| } | ||
|
|
||
| entries, err := ListWorkflows(commit) |
There was a problem hiding this comment.
OK, I think I could understand the problem now.
The proper approach should be like this: add a "WorkflowFilePath" column into the "Actions" model and just use it.
It shouldn't call ListWorkflows for viewing action workflows, otherwise there will be a performance problem.
This Pull Request adds a link in the action run details page that allows a user to quickly access the exact version of the workflow file that was run for the given action. This was requested by users who were actively iterating/debugging actions and needed to see which version of the workflow caused the results they are looking at.
This behavior matches similar behavior in github.
Two challenges implementing this feature were:
To solve this, we take the commit from when the action was run, find the appropriate workflow entry, then traverse up the git tree to determine its relative path at the time the commit was made.