feat!: Add dimensions argument to consumer quota override #683

stanley98yu · 2022-02-01T14:32:32Z

Fixes: #665

I was thinking about adding a test, but I don't have gsuite set up. Is there a non-gsuite-enabled test that would be best?

bharathkkb

Thanks for the PR @stanley98yu
A test would be great. I am not sure what you meant by gsuite as the example inputs only seem to require a project_id.

stanley98yu · 2022-02-02T18:35:14Z

Sorry about the vague question--I misunderstood how the tests are setup.

I've tried adding a new test suite for quota_project, but I'm confused about how Kitchen finds the tests if we don't specify the test location. We specify the fixtures in kitchen.yml, but not the tests in test/integration/controls. Am I missing something? I've pushed my code to show what I've written out so far.

bharathkkb · 2022-02-02T22:17:46Z

@stanley98yu np, it is linked via the test name quota_project which you also have in the inspec.yml for a given test. Since this is a new test, could you use our newer framework? We are trying to phase out the old ruby based tests. New docs are here: https://github.com/GoogleCloudPlatform/cloud-foundation-toolkit/tree/master/infra/blueprint-test
Some example repos are https://github.com/terraform-google-modules/terraform-google-network and https://github.com/terraform-google-modules/terraform-google-event-function

stanley98yu · 2022-02-14T21:16:20Z

Hey @bharathkkb I'm having some trouble using the new test framework. How do I run an arbitrary shell command that isn't gcloud? I think in order to display quotas, I need to curl the Service Consumer Management API endpoint according to: https://cloud.google.com/service-infrastructure/docs/manage-consumer-quota.

I tried looking at https://github.com/GoogleCloudPlatform/cloud-foundation-toolkit/blob/master/infra/blueprint-test/pkg/gcloud/gcloud.go to see how the test commands work, but I'm still new to Go. I've pushed my current test setup if you want to take a look. Let me know your thoughts/alternatives

bharathkkb · 2022-02-14T21:37:01Z

@stanley98yu I'll ping you

bharathkkb · 2022-02-15T04:58:48Z

test/integration/quota_project/quota_project_test.go

+		gcurlCmd := shell.Command{
+			Command: "gcurl",
+			Args:    []string{fmt.Sprintf("https://serviceconsumermanagement.googleapis.com/v1beta1/services/%s/projects/%s/consumerQuotaMetrics", computeAPI, projectID)},
+		}
+		op, err := shell.RunCommandAndGetStdOutE(t, gcurlCmd)
+		if err != nil {
+			t.Fatal(err)
+		}


Lets check if gcloud is available. If not, I am leaning towards using https://pkg.go.dev/golang.org/x/oauth2/google#DefaultClient to make the request. We can add a helper to the test framework if there is verbose logic. Based on the API doc, something like below is what I was thinking

httpClient, err := google.DefaultClient(oauth2.NoContext, "https://www.googleapis.com/auth/cloud-platform") if err != nil { t.Fatalf("",err) } resp, err := httpClient.Get(runEndPointUrl) if err != nil { t.Fatalf("",err) } // use utils.ParseJSONResult on resp.Body

bharathkkb · 2022-02-23T19:02:19Z

@stanley98yu Hoping to pull this into #684 as its a breaking change if you have bandwidth. We can defer the test to a follow up PR if you can validate and add an upgrade guide.

stanley98yu · 2022-02-28T14:43:43Z

@bharathkkb Sorry about the delay, but I don't think I can verify WAI just yet. The dimensions argument doesn't show up in terraform plan, and I'm also still having trouble with the test. I think it may be best to delay adding this feature in until I can put some time to figuring it out. Again, sorry about the inconvenience.

bharathkkb · 2022-03-02T19:37:45Z

@stanley98yu no worries, we can hold for next release. Thanks for checking!

stanley98yu · 2022-03-25T19:48:12Z

I'm able to verify that the feature is WAI but with two notes:

The issue before was that setting the dimensions type constraint as object({}) implies it doesn't have any attributes (and ignores additional ones). dimensions's attributes depend on the specific metric, so I don't know if there's a type constraint that isn't any to match objects with variable attribute names. Is this the correct approach?
I ended up not being able to find a gcloud-equivalent command for calling the Service Usage API, so I stuck with using an HTTP client to call the API.
The checks seem to fail for apply-essential-contacts-example, but I don't think the failures are related to my changes? Note: The test passes when I run manually and passed after re-running.

bharathkkb

Thanks for working on this! Some minor nits related to test below.

The issue before was that setting the dimensions type constraint as object({}) implies it doesn't have any attributes

Baseed on example would map(string) be a better type?

I ended up not being able to find a gcloud-equivalent command for calling the Service Usage API, so I stuck with using an HTTP client to call the API.

Sounds good, this might also be something we can add to the test framework if you are interested in opening an issue here.

The checks seem to fail for apply-essential-contacts-example

Yeah that seemed like a flake.

bharathkkb · 2022-04-06T03:07:11Z

test/integration/quota_project/quota_project_test.go

+
+		projectID := quotaProjectT.GetStringOutput("project_id")
+
+		apis := gcloud.Run(t, fmt.Sprintf("services list --project %s", projectID))


nit: newer versions of the framework has a Runf method which is a bit cleaner

Suggested change

apis := gcloud.Run(t, fmt.Sprintf("services list --project %s", projectID))

apis := gcloud.Runf(t, "services list --project %s", projectID)

bharathkkb · 2022-04-06T03:08:30Z

test/integration/quota_project/quota_project_test.go

+		if err != nil {
+			t.Fatalf("%s", err)
+		}


nit: since we already have assert. https://pkg.go.dev/github.com/stretchr/testify/assert#NoError

Suggested change

if err != nil {

t.Fatalf("%s", err)

}

assert.NoError(err)

here and throughout

bharathkkb · 2022-04-06T03:11:19Z

test/integration/quota_project/quota_project_test.go

+		}
+		result = utils.ParseJSONResult(t, string(body))
+		assert.Equal("95", result.Get("overrides.0.overrideValue").String(), "has correct consumer quota override value")
+		assert.True(!result.Get("overrides.0.dimensions").Exists(), "has correct consumer quota override dimensions")


This seemed a bit confusing - reworded slightly based on fixture

Suggested change

assert.True(!result.Get("overrides.0.dimensions").Exists(), "has correct consumer quota override dimensions")

assert.False(result.Get("overrides.0.dimensions").Exists(), "has empty dimensions")

bharathkkb · 2022-04-06T03:21:32Z

test/integration/quota_project/quota_project_test.go

+		if err != nil {
+			t.Fatalf("%s", err)
+		}
+		body, err := io.ReadAll(resp.Body)


close body to prevent leaks

Suggested change

body, err := io.ReadAll(resp.Body)

defer resp.Body.Close()

body, err := io.ReadAll(resp.Body)

stanley98yu · 2022-04-06T15:16:02Z

Thanks, @bharathkkb! I've updated with the suggestions.

bharathkkb · 2022-04-07T01:44:56Z

hold for next breaking

comment-bot-dev · 2022-04-11T19:32:31Z

@stanley98yu
Thanks for the PR! 🚀
✅ Lint checks have passed.

Add dimensions argument to consumer quota override

184657c

stanley98yu added the breaking label Feb 1, 2022

stanley98yu requested a review from a team as a code owner February 1, 2022 14:32

bharathkkb reviewed Feb 2, 2022

View reviewed changes

Add quota_project test suite

0484723

stanley98yu added 3 commits February 3, 2022 09:33

Merge branch 'master' into dimensions

1b54640

Merge branch 'master' into dimensions

8c6d6da

Setup quota project test draft

8e26ef1

bharathkkb reviewed Feb 15, 2022

View reviewed changes

stanley98yu added 3 commits February 25, 2022 11:23

Merge branch 'master' into dimensions

5009b87

Update version

f8f32e4

Verify WAI

541c432

stanley98yu added 6 commits March 9, 2022 11:52

Merge branch 'master' into dimensions

182bde1

Verify dimensions works and add tests

597a2da

Fix Cloud Build int file

3c976a8

Merge branch 'master' into dimensions

da4dc0b

Forgot to change variable in gsuite_enabled module

cc49cce

Generate docs

e93246b

bharathkkb reviewed Apr 6, 2022

View reviewed changes

Update w/ nit suggestions

93c1bd5

bharathkkb approved these changes Apr 7, 2022

View reviewed changes

Merge branch 'master' into dimensions

4e37cbb

bharathkkb changed the title ~~Add dimensions argument to consumer quota override~~ feat!: Add dimensions argument to consumer quota override Apr 8, 2022

Merge branch 'master' into dimensions

f2ff026

bharathkkb mentioned this pull request Apr 11, 2022

chore(master): release 13.0.0 #693

Merged

bharathkkb merged commit d1d7624 into master Apr 12, 2022

bharathkkb deleted the dimensions branch April 12, 2022 17:55

This was referenced Aug 18, 2022

chore(master): release 14.0.0 #741

Closed

chore(master): release 13.1.0 #713

Closed

release-please bot mentioned this pull request Nov 17, 2022

chore(master): release 15.0.0 #769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Add dimensions argument to consumer quota override #683

feat!: Add dimensions argument to consumer quota override #683

stanley98yu commented Feb 1, 2022

bharathkkb left a comment

stanley98yu commented Feb 2, 2022

bharathkkb commented Feb 2, 2022

stanley98yu commented Feb 14, 2022

bharathkkb commented Feb 14, 2022

bharathkkb Feb 15, 2022 •

edited

bharathkkb commented Feb 23, 2022

stanley98yu commented Feb 28, 2022

bharathkkb commented Mar 2, 2022

stanley98yu commented Mar 25, 2022 •

edited

bharathkkb left a comment

bharathkkb Apr 6, 2022

bharathkkb Apr 6, 2022

bharathkkb Apr 6, 2022

bharathkkb Apr 6, 2022

bharathkkb Apr 6, 2022

stanley98yu commented Apr 6, 2022

bharathkkb commented Apr 7, 2022

comment-bot-dev commented Apr 11, 2022


		projectID := quotaProjectT.GetStringOutput("project_id")

		apis := gcloud.Run(t, fmt.Sprintf("services list --project %s", projectID))

	apis := gcloud.Run(t, fmt.Sprintf("services list --project %s", projectID))
	apis := gcloud.Runf(t, "services list --project %s", projectID)

	assert.True(!result.Get("overrides.0.dimensions").Exists(), "has correct consumer quota override dimensions")
	assert.False(result.Get("overrides.0.dimensions").Exists(), "has empty dimensions")

	body, err := io.ReadAll(resp.Body)
	defer resp.Body.Close()
	body, err := io.ReadAll(resp.Body)

feat!: Add dimensions argument to consumer quota override #683

feat!: Add dimensions argument to consumer quota override #683

Conversation

stanley98yu commented Feb 1, 2022

bharathkkb left a comment

Choose a reason for hiding this comment

stanley98yu commented Feb 2, 2022

bharathkkb commented Feb 2, 2022

stanley98yu commented Feb 14, 2022

bharathkkb commented Feb 14, 2022

bharathkkb Feb 15, 2022 • edited

Choose a reason for hiding this comment

bharathkkb commented Feb 23, 2022

stanley98yu commented Feb 28, 2022

bharathkkb commented Mar 2, 2022

stanley98yu commented Mar 25, 2022 • edited

bharathkkb left a comment

Choose a reason for hiding this comment

bharathkkb Apr 6, 2022

Choose a reason for hiding this comment

bharathkkb Apr 6, 2022

Choose a reason for hiding this comment

bharathkkb Apr 6, 2022

Choose a reason for hiding this comment

bharathkkb Apr 6, 2022

Choose a reason for hiding this comment

bharathkkb Apr 6, 2022

Choose a reason for hiding this comment

stanley98yu commented Apr 6, 2022

bharathkkb commented Apr 7, 2022

comment-bot-dev commented Apr 11, 2022

bharathkkb Feb 15, 2022 •

edited

stanley98yu commented Mar 25, 2022 •

edited