Skip to content
This repository has been archived by the owner on Nov 4, 2022. It is now read-only.

Improve error message for broken symlinks on artifact_path. #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

msuozzo
Copy link
Contributor

@msuozzo msuozzo commented Jul 13, 2021

Fixes #22

Well, it doesn't really fix #22 but it's the best we can do to highlight the issue.

@loosebazooka
Copy link
Contributor

loosebazooka commented Jul 14, 2021

Actually hold on, I'm not sure I know what I'm talking about.

My understanding is that #22 fails on a valid symlink. The file exists, we just can't get to it with filepath.Walk? Would something like https://pkg.go.dev/github.com/facebookgo/symwalk be the way to solve this?

@msuozzo
Copy link
Contributor Author

msuozzo commented Jul 14, 2021

Nah the issue is that we'd be walking from inside the container so the symlink will be broken.

@loosebazooka
Copy link
Contributor

I thought that mark's example actually had a valid file in a valid location because https://github.com/MarkLodato/example-build/blob/main/.github/workflows/build.yaml#L17 still uploaded something?

@MarkLodato
Copy link
Member

The symlink is valid. It's just not visible to the binary running in the container.

I don't think this PR is sufficient to close #22. To improve the error message, we should (1) put a "known issue" on the main page and (2) if we detect that it's a broken symlink, mention that this is a known issue. Just saying "broken symlink" is still going to be confusing since it's not actually broken.

@msuozzo
Copy link
Contributor Author

msuozzo commented Jul 19, 2021

The symlink is valid. It's just not visible to the binary running in the container.

I don't think this distinction is valuable. We're running this action in a container so we have no other view of the system. To us, the symlink is broken.

We could provide more context around why the symlink may be broken but that's pretty much it.

I don't think this PR is sufficient to close #22. To improve the error message, we should (1) put a "known issue" on the main page and (2) if we detect that it's a broken symlink, mention that this is a known issue. Just saying "broken symlink" is still going to be confusing since it's not actually broken.

I think it's a good idea to add it as a known issue.

The symlink breakage is the result of the execution environment but it's broken to the code being run. I'll add more context to the error message but a broken symlink could be either broken by the fs mapping done by docker or broken on the system and we have no way of reliably differentiating between the two.

@MarkLodato
Copy link
Member

Friendly reminder that this PR is still open :-). Probably good to submit even an imperfect solution now while we wait for a better one.

@loosebazooka
Copy link
Contributor

I guess the alternative is: we could rewrite everything in typescript or something that wouldn't have the container sandbox.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Action can't find the artifact
3 participants