-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix: "all_teams" data resource causes 429 status code requests #988
Conversation
/review |
@@ -309,7 +308,7 @@ func TestConfigurationVariableMarshelling(t *testing.T) { | |||
|
|||
b, err := json.Marshal(&variable) | |||
if assert.NoError(t, err) { | |||
assert.False(t, strings.Contains(string(b), str)) | |||
assert.NotContains(t, string(b), str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter complaints...
@@ -0,0 +1,22 @@ | |||
package client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generic memoization function.
@@ -11,6 +11,7 @@ import ( | |||
func dataTeams() *schema.Resource { | |||
return &schema.Resource{ | |||
ReadContext: dataTeamsRead, | |||
Description: "Note: this data source is cached, once fetched it will not be updated until the next plan/apply", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this note so users understand limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some tests are in order I believe. the implementation looks great, but let's make sure stuff are tested properly
It's already covered by some tests (acceptance and harness). |
@TomerHeber I'd like the memoization to be tested at least. to see it doesn't invoke the same call twice for the same parameters, does call for different parameters |
Got it. Makes sense. |
@sagilaufer1992 - added the unit tests for memoization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, my comments were addressed
Issue & Steps to Reproduce / Feature Request
fixes #987
Solution
Added memorization to the get teams call.
This caches results between terraform executions.
The downside is that the results will be stale during the execution.