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

Fix some common artifact download bugs #2878

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Fix some common artifact download bugs #2878

merged 2 commits into from
Jul 8, 2024

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Jul 8, 2024

Description

While investigating the artifact download process a bit more, I discovered some fairly low-hanging fruit in the existing downloader.

Context

Fixes #2774 by making one or another artifact the "winner", and printing a SHA256 sum of the content that it downloaded.

https://coda.io/d/_dHnUHNps1YO#View-3-of-Escalations-Table_tu3KF/r437&view=modal

Changes

  • Write to a temp file and move it to the destination name when ready
  • Check the error return from Close
  • Print a SHA256 sum of the content
  • Add ability to verify SHA256 checksum (not yet used)
  • Make error strings more Gothic

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)

- Write to a temp file and move it to the destination name when ready
- Check the error return from Close
- Print a SHA256 sum of the content
- Add ability to verify SHA256 checksum
- Make error strings more Gothic
@DrJosh9000 DrJosh9000 requested a review from a team July 8, 2024 07:20
Copy link
Contributor

@wolfeidau wolfeidau left a comment

Choose a reason for hiding this comment

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

Just one query.

agent/download.go Show resolved Hide resolved
@DrJosh9000 DrJosh9000 enabled auto-merge July 8, 2024 23:42
@DrJosh9000 DrJosh9000 disabled auto-merge July 8, 2024 23:44
@DrJosh9000 DrJosh9000 enabled auto-merge July 8, 2024 23:44
@DrJosh9000 DrJosh9000 merged commit 52b74d9 into main Jul 8, 2024
1 check passed
@DrJosh9000 DrJosh9000 deleted the artifact-integrity branch July 8, 2024 23:59
@jim-barber-he
Copy link

jim-barber-he commented Jul 19, 2024

This change has resulted in permissions changes of the artifacts where group and world permissions bits are lost/gone.
It used to use os.Create which combines 0666 with the UMASK, while os.CreateTemp will lock permissions down, since temporary files are only accessible by the user creating them.

This caused an outage for us because our application build doesn't make sure the files have appropriate permissions on them.
See:
https://forum.buildkite.community/t/permissions-on-artifacts-changed-with-version-3-75-0-of-buildkite-agent/3803

@DrJosh9000
Copy link
Contributor Author

Thanks for pointing this out @jim-barber-he, the permissions change was definitely unintentional. #2894 to fix it should be in the next release.

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

Successfully merging this pull request may close these issues.

Downloading multiple artifacts with the same file name corrupts them.
3 participants