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

samples(storage transfer): add samples and test cases for storage transfer services #2857

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

Conversation

mahendra-google
Copy link

Adding samples and tests cases for following storage transfer services:

  • 1. storagetransfer_quickstart
  • 2. storagetransfer_transfer_to_nearline
  • 3. storagetransfer_get_latest_transfer_operation
  • 4. storagetransfer_manifest_request
  • 5. storagetransfer_transfer_from_posix
  • 6. storagetransfer_download_to_posix
  • 7. storagetransfer_transfer_posix_to_posix
  • 8. storagetransfer_create_event_driven_gcs_transfer

…utput message in sample output and related small changes.
…sfer to nearline and check latest transfer operation (#1)
…ole , rewording , removing some unnecessary symbols, few test checks removed related to posix file transfer.
@mahendra-google mahendra-google requested review from a team as code owners December 4, 2024 10:22
@product-auto-label product-auto-label bot added api: storagetransfer Issues related to the Storage Transfer Service API. samples Issues that are directly related to samples. labels Dec 4, 2024
@jskeet jskeet assigned amanda-tarafa and unassigned jskeet Dec 4, 2024
@amanda-tarafa amanda-tarafa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2024
Copy link
Member

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

I only reviewed this partially but there are plenty of change requests that will apply to all samples and tests in this PR.

Splitting this PR in several, each container 2 samples and their tests, will make things easier to review.

@@ -6,5 +6,6 @@

<ItemGroup>
<PackageReference Include="Google.Cloud.StorageTransfer.V1" Version="2.7.0" />
<PackageReference Include="xunit.abstractions" Version="2.0.3" />
Copy link
Member

Choose a reason for hiding this comment

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

Why would you need this on the samples project?

Copy link
Author

Choose a reason for hiding this comment

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

I forgot to remove that I added for the output helper. I will remove this.

@@ -25,7 +25,6 @@ public class QuickstartTest : IDisposable
{
private readonly StorageFixture _fixture;
private string _transferJobName;

Copy link
Member

Choose a reason for hiding this comment

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

Revert

@@ -36,8 +35,8 @@ public TransferJob Quickstart(
ProjectId = projectId,
TransferSpec = new TransferSpec
{
GcsDataSink = new GcsData { BucketName = sourceBucket },
GcsDataSource = new GcsData { BucketName = sinkBucket }
GcsDataSink = new GcsData { BucketName = sinkBucket },
Copy link
Member

Choose a reason for hiding this comment

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

It's surprising that the test needed no changes. Are we testing for the right thing?

Copy link
Author

Choose a reason for hiding this comment

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

Yes its just checking transfer job created or not .

public void TransferUsingManifest()
{
TransferUsingManifestSample transferUsingManifestSample = new TransferUsingManifestSample();
var storage = StorageClient.Create();
Copy link
Member

Choose a reason for hiding this comment

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

Don't you have one in the fixture?

Comment on lines 38 to 40
byte[] byteArray = Encoding.UTF8.GetBytes("flower.jpeg");
MemoryStream stream = new MemoryStream(byteArray);
storage.UploadObject(_fixture.BucketNameSource, _fixture.ManifestObjectName, "application/octet-stream", stream);
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the kind of thing that you would add to the fixture, i.e. create s bucket with some data in it that you can use to test transfer jobs.

But, the quickstart was not actually transferring anything. We really don't test the service in samples, we test the samples themselves, so creating and running a transfer job from and empty source seems enough for that.

Copy link
Member

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

mahendra-google and others added 5 commits December 19, 2024 14:49
…estSample.cs

Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
…gManifestTest.cs

Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
@amanda-tarafa
Copy link
Member

@mahendra, there are still conflicts in your branch. Let me know when it's ready for review again, thanks!

@mahendra
Copy link

@mahendra, there are still conflicts in your branch. Let me know when it's ready for review again, thanks!

@amanda-tarafa - I think you meant @mahendra-google and not me.

@amanda-tarafa
Copy link
Member

@mahendra, there are still conflicts in your branch. Let me know when it's ready for review again, thanks!

@amanda-tarafa - I think you meant @mahendra-google and not me.

Yes, you are right, sorry for the noise.

@amanda-tarafa
Copy link
Member

amanda-tarafa commented Dec 20, 2024

Just as a heads up, it's unlikely that I can get to this before January.

I'm running the tests though so we at least know whether things are working or not.

@amanda-tarafa amanda-tarafa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 20, 2024
@kokoro-team kokoro-team removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 20, 2024
@amanda-tarafa
Copy link
Member

Tests are green, thanks. I'll re-review as soon as possible, but likely to be in January.

@amanda-tarafa
Copy link
Member

@mahendra-google There are still a few change requests that haven't been adressed, . Please adresses those, or explain why you shouldn't or ask questions about the change requests if you have them. In particular my comments on the fixture will impact a lot of the test code. I'd prefer to do a pass when things look more definitive. Thanks.

@mahendra-google
Copy link
Author

mahendra-google commented Jan 10, 2025

@mahendra-google There are still a few change requests that haven't been adressed, . Please adresses those, or explain why you shouldn't or ask questions about the change requests if you have them. In particular my comments on the fixture will impact a lot of the test code. I'd prefer to do a pass when things look more definitive. Thanks.

@amanda-tarafa Sorry for delays as I was involved into other high priority tasks that I have been assigned. I will address it soon and will ask questions . Thanks

@amanda-tarafa
Copy link
Member

No rush on my part at all.

@mahendra-google
Copy link
Author

No rush on my part at all.

It's on priority from day 1 and it will be forever. A new resource has been identified for work that I have been assigned I am delegating those tasks to new resource. A KT session is going on. Soon I will be focusing full time on .NET

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storagetransfer Issues related to the Storage Transfer Service API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants