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.

Sign up for GitHub

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

Jump to bottom

Fly excludes Mac metadata when uploading local inputs #8939

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fly excludes Mac metadata when uploading local inputs #8939

wants to merge 4 commits into from

Conversation

selzoc
Copy link
Contributor

@selzoc selzoc commented Apr 5, 2024 •
edited

Changes proposed by this PR

On recent MacOS machines, tar files created (i.e. from fly execute with a local resource) include files prefixed by ._.
As a result, once you transfer this tar file to a Linux system and untar, these files may interfere with jobs. This flag will prevent that from happening.

closes #8916

Release Note

  • fly execute no longer includes MacOS extended file attributes when uploading local inputs

@selzoc selzoc requested a review from a team as a code owner April 5, 2024 22:10
@selzoc selzoc mentioned this pull request Apr 5, 2024
Open
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 5, 2024
View reviewed changes
go-archive/tarfs/extract.go Dismissed Show dismissed Hide dismissed
go-archive/tgzfs/extract.go Dismissed Show dismissed Hide dismissed
go-archive/zipfs/extract.go Dismissed Show dismissed Hide dismissed
go-archive/tarfs/extract.go Dismissed Show dismissed Hide dismissed
go-archive/tarfs/extract.go Dismissed Show dismissed Hide dismissed
go-archive/zipfs/extract.go Dismissed Show dismissed Hide dismissed
taylorsilva
taylorsilva requested changes Apr 5, 2024
View reviewed changes
Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for having the fix in a separate commit to make reviewing easier. I'll figure out this CI and CodeQL stuff.

One thing I'm also not sure about is if our current scripts will run the tests in go-archive or if we'll have to update CI. Going to look into that.

go-archive/NOTICE.md Outdated Show resolved Hide resolved
@taylorsilva taylorsilva added the bug label Apr 5, 2024
@taylorsilva
Copy link
Member

Looks like all the unit tests should get run: https://github.com/concourse/ci/blob/883ba8faf5db4ae8af582143cd7d54f0a59e958b/tasks/scripts/unit#L26

@selzoc
Copy link
Contributor Author

selzoc commented Apr 5, 2024

Looks like all the unit tests should get run: https://github.com/concourse/ci/blob/883ba8faf5db4ae8af582143cd7d54f0a59e958b/tasks/scripts/unit#L26

I'll note that the code that is changed in go-archive has no unit tests. We'd probably have to do a little refactor to pass in a CommandRunner or something if we want to verify the flags passed to tar.

@selzoc
Copy link
Contributor Author

selzoc commented Apr 26, 2024

@taylorsilva any updates here? I'm not able to see the concourse output of the failed ci runs.

@taylorsilva
Copy link
Member

taylorsilva commented Apr 27, 2024 •
edited

@selzoc yes! Could you rebase on master please? We had to fix some stuff in the integration test suite. Once you rebase and CI runs again I'll merge the PR. Thanks for pinging btw

Edit: Forgot I could rebase your PR myself!

taylorsilva
taylorsilva approved these changes Apr 27, 2024
View reviewed changes
Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

Just need CI to pass

selzoc and others added 3 commits April 28, 2024 13:48
@selzoc @taylorsilva
Inline github.com/concourse/go-archive repo
5ec1c6e 
This is in preparation for making a modification to it in relation to
concourse#8916.  The original
repo has been archived for several years.

Signed-off-by: Chris Selzo <[email protected]>
@selzoc @taylorsilva
Add the --no-mac-metadata flag to fly tar commands
b8eb8cf 
On recent MacOS machines, tar files created (i.e. from `fly execute`
with a local resource) include files prefixed by `._`.  As a result,
once you transfer this tar file to a Linux system and untar, these files
may interfere with jobs. This flag will prevent that from happening.

Fixes concourse#8916

See cloudfoundry/bosh-utils@f79167b
for a similar fix to the `bosh` cli.

Signed-off-by: Chris Selzo <[email protected]>
@taylorsilva
remove duplicate LICENSE and NOTICE
a610f2c 
same as the one's in the root of concourse/concourse

Signed-off-by: Taylor Silva <[email protected]>
@taylorsilva
Copy link
Member

taylorsilva commented Apr 28, 2024 •
edited

Ah there's an issue with houdini depending on go-archive still. Can reproduce locally with go test -v ./worker/runtime/integration/

# github.com/concourse/concourse/worker/runtime/integration

../gopath/pkg/mod/github.com/vito/[email protected]/container.go:15:2: missing go.sum entry for module providing package github.com/concourse/go-archive/tarfs (imported by github.com/vito/houdini); to add:

	go get github.com/vito/[email protected]

FAIL	github.com/concourse/concourse/worker/runtime/integration [setup failed]

@selzoc
Copy link
Contributor Author

selzoc commented May 3, 2024

OK I pushed a fix for that integration failure - it's a bit weird to have inlined go-archive and also indirectly reference it, but 🤷

@taylorsilva
Copy link
Member

From the most recent run:

There were failures detected in the following suites:

  tarfs ./go-archive/tarfs [Compilation failure]

  tgzfs ./go-archive/tgzfs [Compilation failure]

  zipfs ./go-archive/zipfs [Compilation failure]


Test Suite Failed

and above that

Failed to compile tarfs:


go: updates to go.mod needed; to update it:

	go mod tidy


Failed to compile tgzfs:


go: updates to go.mod needed; to update it:

	go mod tidy


Failed to compile zipfs:


go: updates to go.mod needed; to update it:

	go mod tidy

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taylorsilva taylorsilva taylorsilva approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fly execute does not exclude MacOS extended file attributes
2 participants
@selzoc @taylorsilva