Improve error message for broken symlinks on artifact_path. by msuozzo · Pull Request #23 · slsa-framework/github-actions-demo · GitHub
Skip to content
This repository was archived by the owner on Nov 4, 2022. It is now read-only.

Improve error message for broken symlinks on artifact_path.#23

Open
msuozzo wants to merge 1 commit into
mainfrom
fix-symlink
Open

Improve error message for broken symlinks on artifact_path.#23
msuozzo wants to merge 1 commit into
mainfrom
fix-symlink

Conversation

@msuozzo

@msuozzo msuozzo commented Jul 13, 2021

Copy link
Copy Markdown
Contributor

Fixes #22

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

@loosebazooka

loosebazooka commented Jul 14, 2021

Copy link
Copy Markdown
Contributor

@msuozzo

msuozzo commented Jul 14, 2021

Copy link
Copy Markdown
Contributor Author

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

@loosebazooka

Copy link
Copy Markdown
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
Copy Markdown
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

msuozzo commented Jul 19, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
Contributor

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