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

Fix object_store docs and Add CI job #4684

Merged
merged 7 commits into from
Aug 11, 2023
Merged

Fix object_store docs and Add CI job #4684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d8f735
Add CI job for object_store_docs
alamb Aug 11, 2023
0793a05
fix job
alamb Aug 11, 2023
422dce3
fix again
alamb Aug 11, 2023
2dcab32
fix
alamb Aug 11, 2023
5ba0579
Fix doc links
alamb Aug 11, 2023
7166e9e
Add comment about why a different workflow is needed
alamb Aug 11, 2023
65f94a3
Fix AmazonS3 link
alamb Aug 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion .github/workflows/object_store.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,24 @@ jobs:
- name: Run clippy with all features and all targets
run: cargo clippy --all-features --all-targets -- -D warnings

# test doc links still work
#
# Note that since object_store is not part of the main workspace,
# this needs a separate docs job as it is not covered by
# `cargo doc --workspace`
docs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should also move the docs workflow into the arrow workflow, as it only applies to that workspace now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a docs workflow that checks the workspace: https://github.com/apache/arrow-rs/blob/master/.github/workflows/docs.yml

However, I think object_store is not part of the main workspace which is why it is not covered:

arrow-rs/Cargo.toml

Lines 20 to 42 in c618438

members = [
"arrow",
"arrow-arith",
"arrow-array",
"arrow-buffer",
"arrow-cast",
"arrow-csv",
"arrow-data",
"arrow-flight",
"arrow-flight/gen",
"arrow-integration-test",
"arrow-integration-testing",
"arrow-ipc",
"arrow-json",
"arrow-ord",
"arrow-row",
"arrow-schema",
"arrow-select",
"arrow-string",
"parquet",
"parquet_derive",
"parquet_derive_test",
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some comments to try and clarify the need

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I agree on the need, I was perhaps suggesting we remove the docs.yml and merge it into arrow.yml

name: Rustdocs
runs-on: ubuntu-latest
defaults:
run:
working-directory: object_store
env:
RUSTDOCFLAGS: "-Dwarnings"
steps:
- uses: actions/checkout@v3
- name: Run cargo doc
run: cargo doc --document-private-items --no-deps --workspace --all-features

# test the crate
# This runs outside a container to workaround lack of support for passing arguments
# to service containers - https://github.com/orgs/community/discussions/26688
Expand Down Expand Up @@ -152,4 +170,4 @@ jobs:
- name: Build wasm32-unknown-unknown
run: cargo build --target wasm32-unknown-unknown
- name: Build wasm32-wasi
run: cargo build --target wasm32-wasi
run: cargo build --target wasm32-wasi
8 changes: 7 additions & 1 deletion object_store/src/aws/copy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@

use crate::config::Parse;

/// Configure how to provide [`ObjectStore::copy_if_not_exists`] for [`AmazonS3`]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than trying to actually make the link to [AmazonS3] work by conditionalizing the docs with the "grim" approach @tustvold describes in #4683 (comment) I opted to simply avoid the link, which I think is reasonable given this is in the aws module and has S3 in the name

/// Configure how to provide [`ObjectStore::copy_if_not_exists`] for
/// [`AmazonS3`].
///
/// [`ObjectStore::copy_if_not_exists`]: crate::ObjectStore::copy_if_not_exists
/// [`AmazonS3`]: super::AmazonS3
#[derive(Debug, Clone)]
#[non_exhaustive]
pub enum S3CopyIfNotExists {
Expand All @@ -32,6 +36,8 @@ pub enum S3CopyIfNotExists {
///
/// For example `header: cf-copy-destination-if-none-match: *`, would set
/// the header `cf-copy-destination-if-none-match` to `*`
///
/// [`ObjectStore::copy_if_not_exists`]: crate::ObjectStore::copy_if_not_exists
Header(String, String),
}

Expand Down
Loading