fix(secret_manager): garbage collect locational secrets in integration tests

[haphungw]

Fixes #4349

The stale secrets were not correctly removed since wrong arguments were passed into delete_secret_by_project_and_location_and_secret. This fix parses the secret name to properly delete stale secrets.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.98%. Comparing base (fc20328) to head (be14874).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4433      +/-   ##
==========================================
- Coverage   94.99%   94.98%   -0.02%     
==========================================
  Files         193      193              
  Lines        7294     7294              
==========================================
- Hits         6929     6928       -1     
- Misses        365      366       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coryan]

Good find! I think we can improve the parsing and prevent some panics.

[coryan]

This will panic if the name is not in the right format:

https://doc.rust-lang.org/std/vec/struct.Vec.html#indexing

You may try something like:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c1164c67bd2b89c1fcde61bcd31ed939

Which validates all the stuff and never panics.

[coryan]

I was maybe too succinct in my playground example:

Suggested change

	fn extract(name: &str) -> Result<(&str, &str, &str)> {
	fn parse_secret_name(name: &str) -> Result<(&str, &str, &str)> {

[coryan]

Consider:

Suggested change

	#[test]
	fn test_extract_invalid_name() {
	let name = "invalid/format";
	assert!(extract(name).is_err());
	}
	#[test]
	fn test_extract_missing_parts() {
	assert!(extract("projects/only-one").is_err());
	}
	#[test_case("invalid/format")]
	#[test_case("projects/only-one")]
	#[test_case("projects/p/locations/l/bad/s")]
	#[test_case("projects/p/bad/l/secrets/s")]
	#[test_case("bad/p/locations/l/secrets/s")]
	fn test_extract_invalid_name(input: &str) {
	let result = parse_secret_name(input);
	assert!(result.is_err(), "{result:?});
	}

[coryan]

This still panics, no? Maybe something like (but move the function definition to the right place):

async fn delete_by_name(client: smo::client::SecretManagerService, &name: &str) -> anyhow::Result<()> {
    let (project, location, secret) = parse_secret_name(name)?;
    client.delete_secret_by_project_and_location_and_secret()
        .set_project(project)
        .set_location(location)
        .set_secret(secret)
        .with_idempotency(true)
        .send()
        .await?;    
    Ok(())
}

let pending = stale_secrets.iter().map(|name| delete_by_name(name));

[coryan]

Hmmm... now that I think about it, we don't need a function, I think an expression like this should work to:

let pending = stale_secrets.iter().map(|name| -> anyhow::Result<()> {
    let (project, location, secret) = parse_secret_name(name)?;
    client.delete_secret_by_project_and_location_and_secret()
        .set_project(project)
        .set_location(location)
        .set_secret(secret)
        .with_idempotency(true)
        .send()
        .await?;    
    Ok(())    
});

Your call.

[dbolduc]

Looks great!

The build failure is unrelated... I think it will magically get fixed after
(1) we publish the google-cloud-storage crate at version 1.7.0
(2) we rerun the semver checks build