Skip to content

fix: ghac doesn't support delete anymore #5628

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

Merged
merged 5 commits into from
Feb 14, 2025
Merged

fix: ghac doesn't support delete anymore #5628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae738ba
fix: ghac doesn't support delete anymore
Xuanwo Feb 14, 2025
f66226b
Merge branch 'main' into ghac-v2
Xuanwo Feb 14, 2025
5e928c0
Fix tests
Xuanwo Feb 14, 2025
6044611
Fix tests
Xuanwo Feb 14, 2025
ccf4ea1
Fix tests
Xuanwo Feb 14, 2025
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
2 changes: 1 addition & 1 deletion .github/workflows/service_test_ghac.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ jobs:
- name: Test
shell: bash
working-directory: core
run: cargo nextest run behavior --features tests,services-ghac
run: cargo test behavior --features tests,services-ghac
Copy link
Member

Choose a reason for hiding this comment

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

May I ask what's the rationale here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nextest relies on path/to/test/binary list to enumerate all test cases. If any logs are output at this stage, it will refuse to run.

Output likes:

error: creating test list failed

Caused by:
  for `opendal::behavior`, line '  2025-02-14T11:57:30.185063Z DEBUG opendal::services: service=ghac name=opendal: metadata started' did not end with the string ': test' or ': benchmark'
full output:
behavior::test_read_full: test
behavior::test_read_range: test
behavior::test_reader: test
behavior::test_reader_with_if_match: test
behavior::test_reader_with_if_none_match: test

env:
OPENDAL_TEST: ghac
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
49 changes: 1 addition & 48 deletions core/src/services/ghac/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,13 @@ use http::header::AUTHORIZATION;
use http::header::CONTENT_LENGTH;
use http::header::CONTENT_RANGE;
use http::header::CONTENT_TYPE;
use http::header::USER_AGENT;
use http::Request;
use http::Response;
use http::StatusCode;
use log::debug;
use serde::Deserialize;
use serde::Serialize;

use super::delete::GhacDeleter;
use super::error::parse_error;
use super::writer::GhacWriter;
use crate::raw::*;
Expand All @@ -54,14 +52,6 @@ const ACTIONS_CACHE_URL: &str = "ACTIONS_CACHE_URL";
/// This token will be valid for 6h and github action will running for 6
/// hours at most. So we don't need to refetch it again.
const ACTIONS_RUNTIME_TOKEN: &str = "ACTIONS_RUNTIME_TOKEN";
/// The token provided by workflow;
const GITHUB_TOKEN: &str = "GITHUB_TOKEN";
/// The github api url for ghac.
const GITHUB_API_URL: &str = "GITHUB_API_URL";
/// The repository that runs this action.
const GITHUB_REPOSITORY: &str = "GITHUB_REPOSITORY";
/// The github API version that used by OpenDAL.
const GITHUB_API_VERSION: &str = "2022-11-28";

fn value_or_env(
explicit_value: Option<String>,
Expand Down Expand Up @@ -198,11 +188,6 @@ impl Builder for GhacBuilder {
.clone()
.unwrap_or_else(|| "opendal".to_string()),

api_url: env::var(GITHUB_API_URL)
.unwrap_or_else(|_| "https://api.github.com".to_string()),
api_token: env::var(GITHUB_TOKEN).unwrap_or_default(),
repo: env::var(GITHUB_REPOSITORY).unwrap_or_default(),

client,
};

Expand All @@ -220,18 +205,14 @@ pub struct GhacBackend {
catch_token: String,
version: String,

api_url: String,
pub api_token: String,
repo: String,

pub client: HttpClient,
}

impl Access for GhacBackend {
type Reader = HttpBody;
type Writer = GhacWriter;
type Lister = ();
type Deleter = oio::OneShotDeleter<GhacDeleter>;
type Deleter = ();
type BlockingReader = ();
type BlockingWriter = ();
type BlockingLister = ();
Expand All @@ -258,7 +239,6 @@ impl Access for GhacBackend {

write: true,
write_can_multi: true,
delete: true,

shared: true,

Expand Down Expand Up @@ -356,13 +336,6 @@ impl Access for GhacBackend {

Ok((RpWrite::default(), GhacWriter::new(self.clone(), cache_id)))
}

async fn delete(&self) -> Result<(RpDelete, Self::Deleter)> {
Ok((
RpDelete::default(),
oio::OneShotDeleter::new(GhacDeleter::new(self.clone())),
))
}
}

impl GhacBackend {
Expand Down Expand Up @@ -463,26 +436,6 @@ impl GhacBackend {

Ok(req)
}

pub async fn ghac_delete(&self, path: &str) -> Result<Response<Buffer>> {
let p = build_abs_path(&self.root, path);

let url = format!(
"{}/repos/{}/actions/caches?key={}",
self.api_url,
self.repo,
percent_encode_path(&p)
);

let mut req = Request::delete(&url);
req = req.header(AUTHORIZATION, format!("Bearer {}", self.api_token));
req = req.header(USER_AGENT, format!("opendal/{VERSION} (service ghac)"));
req = req.header("X-GitHub-Api-Version", GITHUB_API_VERSION);

let req = req.body(Buffer::new()).map_err(new_request_build_error)?;

self.client.send(req).await
}
}

#[derive(Deserialize)]
Expand Down
52 changes: 0 additions & 52 deletions core/src/services/ghac/delete.rs
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions core/src/services/ghac/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
// specific language governing permissions and limitations
// under the License.

#[cfg(feature = "services-ghac")]
mod delete;
#[cfg(feature = "services-ghac")]
mod error;
#[cfg(feature = "services-ghac")]
Expand Down
40 changes: 9 additions & 31 deletions core/tests/behavior/blocking_copy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,11 @@ pub fn tests(op: &Operator, tests: &mut Vec<Trial>) {

/// Copy a file and test with stat.
pub fn test_blocking_copy_file(op: BlockingOperator) -> Result<()> {
let source_path = uuid::Uuid::new_v4().to_string();
let (source_content, _) = gen_bytes(op.info().full_capability());
let (source_path, source_content, _) = TEST_FIXTURE.new_file(op.clone());

op.write(&source_path, source_content.clone())?;

let target_path = uuid::Uuid::new_v4().to_string();
let target_path = TEST_FIXTURE.new_file_path();

op.copy(&source_path, &target_path)?;

Expand All @@ -55,8 +54,6 @@ pub fn test_blocking_copy_file(op: BlockingOperator) -> Result<()> {
format!("{:x}", Sha256::digest(&source_content)),
);

op.delete(&source_path).expect("delete must succeed");
op.delete(&target_path).expect("delete must succeed");
Ok(())
}

Expand Down Expand Up @@ -96,46 +93,37 @@ pub fn test_blocking_copy_target_dir(op: BlockingOperator) -> Result<()> {
return Ok(());
}

let source_path = uuid::Uuid::new_v4().to_string();
let (source_content, _) = gen_bytes(op.info().full_capability());
let (source_path, source_content, _) = TEST_FIXTURE.new_file(op.clone());

op.write(&source_path, source_content)?;

let target_path = format!("{}/", uuid::Uuid::new_v4());
let target_path = TEST_FIXTURE.new_dir_path();

op.create_dir(&target_path)?;

let err = op
.copy(&source_path, &target_path)
.expect_err("copy must fail");
assert_eq!(err.kind(), ErrorKind::IsADirectory);

op.delete(&source_path).expect("delete must succeed");
op.delete(&target_path).expect("delete must succeed");
Ok(())
}

/// Copy a file to self should return an error.
pub fn test_blocking_copy_self(op: BlockingOperator) -> Result<()> {
let source_path = uuid::Uuid::new_v4().to_string();
let (source_content, _size) = gen_bytes(op.info().full_capability());
let (source_path, source_content, _) = TEST_FIXTURE.new_file(op.clone());

op.write(&source_path, source_content)?;

let err = op
.copy(&source_path, &source_path)
.expect_err("copy must fail");
assert_eq!(err.kind(), ErrorKind::IsSameFile);

op.delete(&source_path).expect("delete must succeed");
Ok(())
}

/// Copy to a nested path, parent path should be created successfully.
pub fn test_blocking_copy_nested(op: BlockingOperator) -> Result<()> {
let source_path = uuid::Uuid::new_v4().to_string();
let (source_content, _) = gen_bytes(op.info().full_capability());

let (source_path, source_content, _) = TEST_FIXTURE.new_file(op.clone());
op.write(&source_path, source_content.clone())?;

let target_path = format!(
Expand All @@ -144,6 +132,7 @@ pub fn test_blocking_copy_nested(op: BlockingOperator) -> Result<()> {
uuid::Uuid::new_v4(),
uuid::Uuid::new_v4()
);
TEST_FIXTURE.add_path(target_path.clone());

op.copy(&source_path, &target_path)?;

Expand All @@ -152,23 +141,15 @@ pub fn test_blocking_copy_nested(op: BlockingOperator) -> Result<()> {
format!("{:x}", Sha256::digest(target_content)),
format!("{:x}", Sha256::digest(&source_content)),
);

op.delete(&source_path).expect("delete must succeed");
op.delete(&target_path).expect("delete must succeed");
Ok(())
}

/// Copy to a exist path should overwrite successfully.
pub fn test_blocking_copy_overwrite(op: BlockingOperator) -> Result<()> {
let source_path = uuid::Uuid::new_v4().to_string();
let (source_content, _) = gen_bytes(op.info().full_capability());

let (source_path, source_content, _) = TEST_FIXTURE.new_file(op.clone());
op.write(&source_path, source_content.clone())?;

let target_path = uuid::Uuid::new_v4().to_string();
let (target_content, _) = gen_bytes(op.info().full_capability());
assert_ne!(source_content, target_content);

let (target_path, target_content, _) = TEST_FIXTURE.new_file(op.clone());
op.write(&target_path, target_content)?;

op.copy(&source_path, &target_path)?;
Expand All @@ -178,8 +159,5 @@ pub fn test_blocking_copy_overwrite(op: BlockingOperator) -> Result<()> {
format!("{:x}", Sha256::digest(target_content)),
format!("{:x}", Sha256::digest(&source_content)),
);

op.delete(&source_path).expect("delete must succeed");
op.delete(&target_path).expect("delete must succeed");
Ok(())
}
9 changes: 2 additions & 7 deletions core/tests/behavior/blocking_create_dir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,12 @@ pub fn test_blocking_create_dir(op: BlockingOperator) -> Result<()> {
return Ok(());
}

let path = format!("{}/", uuid::Uuid::new_v4());
let path = TEST_FIXTURE.new_dir_path();

op.create_dir(&path)?;

let meta = op.stat(&path)?;
assert_eq!(meta.mode(), EntryMode::DIR);

op.delete(&path).expect("delete must succeed");
Ok(())
}

Expand All @@ -54,15 +52,12 @@ pub fn test_blocking_create_dir_existing(op: BlockingOperator) -> Result<()> {
return Ok(());
}

let path = format!("{}/", uuid::Uuid::new_v4());

let path = TEST_FIXTURE.new_dir_path();
op.create_dir(&path)?;

op.create_dir(&path)?;

let meta = op.stat(&path)?;
assert_eq!(meta.mode(), EntryMode::DIR);

op.delete(&path).expect("delete must succeed");
Ok(())
}
10 changes: 6 additions & 4 deletions core/tests/behavior/blocking_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub fn test_blocking_list_dir(op: BlockingOperator) -> Result<()> {
let parent = uuid::Uuid::new_v4().to_string();
let path = format!("{parent}/{}", uuid::Uuid::new_v4());
debug!("Generate a random file: {}", &path);
let (content, size) = gen_bytes(op.info().full_capability());
let (_, content, size) = TEST_FIXTURE.new_file_with_path(op.clone(), &path);

op.write(&path, content).expect("write must succeed");

Expand All @@ -61,14 +61,12 @@ pub fn test_blocking_list_dir(op: BlockingOperator) -> Result<()> {
}
}
assert!(found, "file should be found in list");

op.delete(&path).expect("delete must succeed");
Ok(())
}

/// List non exist dir should return nothing.
pub fn test_blocking_list_non_exist_dir(op: BlockingOperator) -> Result<()> {
let dir = format!("{}/", uuid::Uuid::new_v4());
let dir = TEST_FIXTURE.new_dir_path();

let obs = op.lister(&dir)?;
let mut objects = HashMap::new();
Expand All @@ -84,6 +82,10 @@ pub fn test_blocking_list_non_exist_dir(op: BlockingOperator) -> Result<()> {

// Remove all should remove all in this path.
pub fn test_blocking_remove_all(op: BlockingOperator) -> Result<()> {
if !op.info().full_capability().delete {
return Ok(());
}

let parent = uuid::Uuid::new_v4().to_string();

let expected = [
Expand Down
Loading
Loading