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

Service: adding ocios support #4189

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Service: adding ocios support #4189

wants to merge 1 commit into from

Conversation

zwpaper
Copy link
Contributor

@zwpaper zwpaper commented Feb 17, 2024

currently support get, list

currently support get, list

Signed-off-by: Wei Zhang <[email protected]>
@@ -249,6 +251,8 @@ impl Scheme {
Scheme::Postgresql,
#[cfg(feature = "services-gdrive")]
Scheme::Gdrive,
#[cfg(feature = "services-ocios")]
Scheme::OciOs,
Copy link
Member

Choose a reason for hiding this comment

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

Please use Ocios instead. We following the same pattern across the whole project:

  • Ocios -> ocios
  • OciOs -> oci_os


let loader = OCILoader::default();

let signer = OCIAPIKeySigner::default();
Copy link
Member

Choose a reason for hiding this comment

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

Will we have different signer from oci?

})?
};

let loader = OCILoader::default();
Copy link
Member

Choose a reason for hiding this comment

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

Please use OciLoader instead. Maybe need changes from reqsign.

/// Oracle Cloud Infrastructure Object Storage support
#[doc = include_str!("docs.md")]
#[derive(Default)]
pub struct OciOsBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Please add an OciosConfig.

read_with_if_match: true,
read_with_if_none_match: true,

write: false,
Copy link
Member

Choose a reason for hiding this comment

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

Please don't declare false.

if let Some(cred) = cred {
Ok(Some(cred))
} else {
// Mark this error as temporary since it could be caused by Aliyun STS.
Copy link
Member

Choose a reason for hiding this comment

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

wrong comment.

) -> Result<Request<AsyncBody>> {
let p = build_abs_path(&self.root, path);
let url = format!(
"objectstorage.{}.oraclecloud.com/n/{}/b/{}/o/{}",
Copy link
Member

Choose a reason for hiding this comment

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

url without protocol could be wrong.

let p = build_abs_path(&self.root, path);
let url = format!(
"objectstorage.{}.oraclecloud.com/n/{}/b/{}/o",
self.region, self.namespace, self.bucket,
Copy link
Member

Choose a reason for hiding this comment

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

If the endpoint is always the same for all API, we can build this endpoint during build.


// Add query arguments to the URL based on response overrides
let mut query_args = Vec::new();
query_args.push(format!("{}={}", constants::QUERY_LIST_PREFIXY, p,));
Copy link
Member

Choose a reason for hiding this comment

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

query_args seems not used?

let bs = bytes::Bytes::from(
r#"
<?xml version="1.0" ?>
<Error xmlns="http://doc.oss-cn-hangzhou.aliyuncs.com">
Copy link
Member

Choose a reason for hiding this comment

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

Is this test correct?

@@ -279,7 +285,7 @@ sha1 = { version = "0.10.6", optional = true }
sha2 = { version = "0.10", optional = true }

# For http based services.
reqsign = { version = "0.14.7", default-features = false, optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Pin it to a commit should be better?

Copy link
Member

Choose a reason for hiding this comment

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

We will continue after reqsign making a new 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.

3 participants