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

feat: Add S3 support #1008

Open
wants to merge 66 commits into
base: main
Choose a base branch
from
Open

feat: Add S3 support #1008

wants to merge 66 commits into from

Conversation

pavelzw
Copy link
Contributor

@pavelzw pavelzw commented Jan 2, 2025

Description

xref #960

#960 (comment)

crates/rattler_networking/src/s3_middleware.rs Outdated Show resolved Hide resolved
crates/rattler_networking/src/s3_middleware.rs Outdated Show resolved Hide resolved
crates/rattler_networking/src/s3_middleware.rs Outdated Show resolved Hide resolved
crates/rattler_networking/src/s3_middleware.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a current limitation is that we need to have one shared credential for each bucket we access with this setup.
we could also make the config-file and profile channel-dependent but i think as a first version, this is okay, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you would add specific config per channel what would that look like? Does it change the API much?

Copy link

Choose a reason for hiding this comment

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

we could also make the config-file and profile channel-dependent

I think this comment was based on an outdated version of the code. Still, we do not support channel-specific configuration yet. Did you already think about an API @pavelzw?

@pavelzw pavelzw marked this pull request as ready for review January 2, 2025 19:10
@pavelzw pavelzw marked this pull request as draft January 3, 2025 11:38
Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Thanks everyone for this PR, I left some small comments but overall this looks great!

.config/nextest.toml Outdated Show resolved Hide resolved
# - { name: "Linux-mips", target: mips-unknown-linux-musl, os: ubuntu-latest, use-cross: true, skip-tests: true }
# - { name: "Linux-mipsel", target: mipsel-unknown-linux-musl, os: ubuntu-latest, use-cross: true, skip-tests: true }
# - { name: "Linux-mips64", target: mips64-unknown-linux-muslabi64, os: ubuntu-latest, use-cross: true, skip-tests: true }
# - { name: "Linux-mips64el", target: mips64el-unknown-linux-muslabi64, os: ubuntu-latest, use-cross: true, skip-tests: true }

# - { name: "Linux-powerpc", target: powerpc-unknown-linux-gnu, os: ubuntu-latest, use-cross: true, skip-tests: true }
# - { name: "Linux-powerpc", target: powerpc-unknown-linux-gnu, os: ubuntu-latest, use-cross: true, skip-tests: true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be reverted?

Copy link

Choose a reason for hiding this comment

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

imo it's cleaner now (my yaml linter fixed this), since it's consistent with the previous line comments.

- { name: "Linux-powerpc64", target: powerpc64-unknown-linux-gnu, os: ubuntu-latest, use-cross: true, skip-tests: true }
- { name: "Linux-powerpc64le", target: powerpc64le-unknown-linux-gnu, os: ubuntu-latest, use-cross: true, skip-tests: true }

- { name: "Linux-s390x", target: s390x-unknown-linux-gnu, os: ubuntu-latest, use-cross: true, skip-tests: true }

- { name: "macOS-x86_64", target: x86_64-apple-darwin, os: macOS-latest }
- { name: "macOS-aarch64", target: aarch64-apple-darwin, os: macOS-latest, skip-tests: true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be reverted?

Copy link

Choose a reason for hiding this comment

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

@pavelzw activated the tests on macOS-aarch64; is there a reason not to execute them?

Cargo.toml Outdated Show resolved Hide resolved
crates/rattler/src/cli/auth.rs Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you would add specific config per channel what would that look like? Does it change the API much?

crates/rattler_networking/src/s3_middleware.rs Outdated Show resolved Hide resolved
crates/rattler_networking/src/s3_middleware.rs Outdated Show resolved Hide resolved
crates/rattler_networking/src/s3_middleware.rs Outdated Show resolved Hide resolved
@wolfv
Copy link
Contributor

wolfv commented Jan 6, 2025

Very nice work @pavelzw @delsner ! :)

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.

4 participants