feat(storageinsights): Adds samples for storage insights #2612

JesseLovelace · 2024-04-29T16:52:50Z

Adds samples for storage insights, and tests

snippet-bot · 2024-04-29T16:52:55Z

Here is the summary of changes.

You are about to add 5 region tags.

storageinsights/api/StorageInsights.Samples/CreateInventoryReportConfig.cs:15, tag storageinsights_create_inventory_report_config
storageinsights/api/StorageInsights.Samples/DeleteInventoryReportConfig.cs:15, tag storageinsights_delete_inventory_report_config
storageinsights/api/StorageInsights.Samples/EditInventoryReportConfig.cs:15, tag storageinsights_edit_inventory_report_config
storageinsights/api/StorageInsights.Samples/GetInventoryReportNames.cs:15, tag storageinsights_get_inventory_report_names
storageinsights/api/StorageInsights.Samples/ListInventoryReportConfigs.cs:15, tag storageinsights_list_inventory_report_configs

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

amanda-tarafa

For tests to run on CI you need to add a file such as this one:

https://github.com/GoogleCloudPlatform/dotnet-docs-samples/blob/main/storage/api/runTests.ps1

amanda-tarafa

A few change requests but nothing major. Mostly idiomatic things.

Some of the comments will apply to everything although I've only brought them up in a couple of places.

amanda-tarafa · 2024-05-07T06:37:53Z

storageinsights/api/StorageInsights.Samples/ListInventoryReportConfigs.cs

+using System;
+using System.Collections.Generic;
+
+namespace StorageInsights.Samples


We don't use namespace in samples because of the extra indentation and verbosity. We do use them in tests though.

Or now, with C# 8, you can just leave the namespace declaration but remove the associated brackets and indent.

Here and everywhere.

amanda-tarafa · 2024-05-07T06:40:15Z

storageinsights/api/StorageInsights.Samples.Tests/ListInventoryReportConfigsTest.cs

+        RetryRobot robot = new RetryRobot() { ShouldRetry = (e) => true };
+        robot.Eventually(() =>
+        {
+            var reportConfig = new CreateInventoryReportConfigSample().CreateInventoryReportConfig(_fixture.ProjectId,


I believe this is what would fail because of permissions, so just have this within the retry. And also a note explaining why.

amanda-tarafa · 2024-05-07T06:41:08Z

storageinsights/api/StorageInsights.Samples.Tests/ListInventoryReportConfigsTest.cs

+    [Fact]
+    public void TestListInventoryReportConfigs()
+    {
+        RetryRobot robot = new RetryRobot() { ShouldRetry = (e) => true };


Just retry for the permissions related exceptions? Else we risk things timing out when there's a different error, and then we don't know what exactly happened.

amanda-tarafa · 2024-05-07T06:41:46Z

storageinsights/api/StorageInsights.Samples.Tests/ListInventoryReportConfigsTest.cs

+            bool found = false;
+
+            foreach (var config in reportConfigs)
+            {
+                if (config.Name.Contains(reportConfig.Name.Split("/")[5]))
+                {
+                    found = true;
+                }
+            }
+
+            Assert.True(found);


User Assert.Contains instead.

I can't do that here unfortunately, the responses from CreateInventoryReportConfig And ListInventoryReportConfigs are slightly different. The Create response has the project ID in the resource name (so like "projects/projectID/locations/...", but the List responses have the project number there instead (so like "projects/1234/locations/..."

So reportConfigs.contains(reportConfig) actually returns false here. That's why I just search every element to see if it contains the report config name component instead

You can still use with a condition, something like (I'm doing this by heart so it may have syntax errors):

Assert.Contains(reportConfigs, config => config.Name.Contains(reportConfig.NameAsResourceName.Name));

amanda-tarafa · 2024-05-07T06:42:49Z

storageinsights/api/StorageInsights.Samples.Tests/ListInventoryReportConfigsTest.cs

+
+            foreach (var config in reportConfigs)
+            {
+                if (config.Name.Contains(reportConfig.Name.Split("/")[5]))


You can probably use reportConfig.NameAsResourceName to access the segment you need to access without having to manipulate strings.

amanda-tarafa · 2024-05-07T06:49:39Z

storageinsights/api/StorageInsights.Samples.Tests/GetInventoryReportNamesTest.cs

+    [Fact]
+    public void TestGetInventoryReportNames()
+    {
+        RetryRobot robot = new RetryRobot() { ShouldRetry = (e) => true };


Only retry the permission related exceptions. Here and elsewhere.

amanda-tarafa · 2024-05-07T06:50:21Z

storageinsights/api/StorageInsights.Samples.Tests/GetInventoryReportNamesTest.cs

+            GetInventoryReportNamesSample sample = new GetInventoryReportNamesSample();
+            sample.GetInventoryReportNames(_fixture.ProjectId, _fixture.BucketLocation,
+                reportConfig.Name.Split("/")[5]);
+            /* We can't actually test for a report config name showing up here, because we create


This is fine, but if you are not using the list with the report names, do not return anything from the sample.

amanda-tarafa · 2024-05-07T06:50:46Z

storageinsights/api/StorageInsights.Samples/EditInventoryReportConfig.cs

+        public ReportConfig EditInventoryReportConfig(
+            string projectId = "your-project-id",
+            string bucketLocation = "us-west-1",
+            string inventoryReportConfigUuid = "2b90d21c-f2f4-40b5-9519-e29a78f2b09f")


Same as before.

amanda-tarafa · 2024-05-07T06:54:05Z

storageinsights/api/StorageInsights.Samples.Tests/DeleteInventoryReportConfigTest.cs

+            foreach (ReportConfig report in _fixture.StorageInsights.ListReportConfigs(
+                         LocationName.Format(_fixture.ProjectId, _fixture.BucketLocation)))
+            {
+                Assert.NotEqual(report.Name, reportConfig.Name);
+            }


Assert.DoesNotContain

Or better, check that getting the report throws with a NOT_FOUND.

amanda-tarafa · 2024-05-07T06:58:46Z

storageinsights/api/StorageInsights.Samples/CreateInventoryReportConfig.cs

+                FrequencyOptions = new FrequencyOptions
+                {
+                    Frequency = FrequencyOptions.Types.Frequency.Weekly,
+                    StartDate = new Date{ Year = 3022, Month = 11, Day = 15 },


Is this year intentional? Can we use a year that's in the 2020's?

The time has to be some minimum time in the future (not sure the exact constraint), and I also wanted to make sure that in the case of a report config accidentally not getting deleted, we don't waste resources by actually making reports. So i just picked a date that would always be in the future and wouldn't have to be updated whenever that year eventually passes.

I guess we could just do something like make it today's date plus one or something, would you prefer that?

Yes, I think so, and if you can find that minimum and use that to move now into the future, with a note explaining, even better. So users know that they at least need to use that minimum as well.

amanda-tarafa · 2024-05-08T07:12:35Z

storageinsights/api/StorageInsights.Samples.Tests/ListInventoryReportConfigsTest.cs

+            bool found = false;
+
+            foreach (var config in reportConfigs)
+            {
+                if (config.Name.Contains(reportConfig.Name.Split("/")[5]))
+                {
+                    found = true;
+                }
+            }
+
+            Assert.True(found);


You can still use with a condition, something like (I'm doing this by heart so it may have syntax errors):

Assert.Contains(reportConfigs, config => config.Name.Contains(reportConfig.NameAsResourceName.Name));

amanda-tarafa · 2024-05-08T07:14:34Z

storageinsights/api/StorageInsights.Samples/CreateInventoryReportConfig.cs

+                FrequencyOptions = new FrequencyOptions
+                {
+                    Frequency = FrequencyOptions.Types.Frequency.Weekly,
+                    StartDate = new Date{ Year = 3022, Month = 11, Day = 15 },


Yes, I think so, and if you can find that minimum and use that to move now into the future, with a note explaining, even better. So users know that they at least need to use that minimum as well.

amanda-tarafa · 2024-05-08T07:17:51Z

storageinsights/api/StorageInsights.Samples/GetInventoryReportNames.cs

+                for (long index = reportDetail.ShardsCount - 1; index >= 0; index--)
+                {
+                    String reportName = reportDetail.ReportPathPrefix + index + "." + extension;
+                    reportNames.Add(reportName);


Oh I meant adding the calculate reportNames to a list to return that list. Yes of course you have to show how to calculate the report names, and then you print them in the console for demonstration purposes. What's odd is to also add them to a list. Some users would want them on a list, some users will write them to a file, some users will inmediately start processes to download the reports etc... So, I just meant stop building and returning the list with all the reports in it. Adn you are not using this for testing so that's fine.

amanda-tarafa

A few, mostly style changes. But looking good.

storageinsights/api/StorageInsights.Samples/GetInventoryReportNames.cs

storageinsights/api/StorageInsights.Samples/EditInventoryReportConfig.cs

amanda-tarafa · 2024-05-09T18:10:44Z

storageinsights/api/StorageInsights.Samples.Tests/EditInventoryReportConfigTest.cs

+        });
+        EditInventoryReportConfigSample sample = new EditInventoryReportConfigSample();
+        var updatedConfig = sample.EditInventoryReportConfig(_fixture.ProjectId, _fixture.BucketLocation,
+            reportConfig.Name.Split("/")[5]);


There should be a reportConfig.NameAsResourceName or similar that you can use so that you don't need to manipulate the string.

storageinsights/api/StorageInsights.Samples/CreateInventoryReportConfig.cs

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

amanda-tarafa

LGMT, thanks!

feat(storageinsights)Adds samples for storage insights

cbdf9c6

JesseLovelace requested a review from a team as a code owner April 29, 2024 16:52

product-auto-label bot added the samples Issues that are directly related to samples. label Apr 29, 2024

blunderbuss-gcf bot assigned jskeet Apr 29, 2024

jskeet assigned amanda-tarafa and unassigned jskeet Apr 29, 2024

amanda-tarafa requested changes Apr 29, 2024

View reviewed changes

JesseLovelace added 2 commits April 30, 2024 13:52

add tests script

d5ace0f

fix xunit version

a4f5263

JesseLovelace added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

JesseLovelace added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

JesseLovelace added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

Add retries to tests

7050fa0

JesseLovelace added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024

amanda-tarafa requested changes May 7, 2024

View reviewed changes

amanda-tarafa requested changes May 8, 2024

View reviewed changes

review comments

e1bb220

amanda-tarafa reviewed May 9, 2024

View reviewed changes

JesseLovelace and others added 2 commits May 9, 2024 13:33

Apply suggestions from code review

7e2f70f

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

last adjustments

ec1a30b

amanda-tarafa approved these changes May 9, 2024

View reviewed changes

amanda-tarafa merged commit c392a20 into main May 9, 2024
10 checks passed

amanda-tarafa deleted the insights branch May 9, 2024 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storageinsights): Adds samples for storage insights #2612

feat(storageinsights): Adds samples for storage insights #2612

JesseLovelace commented Apr 29, 2024

snippet-bot bot commented Apr 29, 2024 •

edited

amanda-tarafa left a comment

amanda-tarafa left a comment

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

JesseLovelace May 7, 2024

amanda-tarafa May 8, 2024

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

amanda-tarafa May 7, 2024

JesseLovelace May 7, 2024

amanda-tarafa May 8, 2024

amanda-tarafa May 8, 2024

amanda-tarafa May 8, 2024

amanda-tarafa May 8, 2024

amanda-tarafa left a comment

amanda-tarafa May 9, 2024

amanda-tarafa left a comment

feat(storageinsights): Adds samples for storage insights #2612

feat(storageinsights): Adds samples for storage insights #2612

Conversation

JesseLovelace commented Apr 29, 2024

snippet-bot bot commented Apr 29, 2024 • edited

amanda-tarafa left a comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

snippet-bot bot commented Apr 29, 2024 •

edited