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

Add query option to agent pool list options #417

Merged
merged 2 commits into from Jul 13, 2022

Conversation

JarrettSpiker
Copy link
Contributor

@JarrettSpiker JarrettSpiker commented May 31, 2022

Description

Add query option to agent pool list options

This will be blocked until the agent pool search feature is enabled (feature flag: remove-agent-pool-limit)

Testing plan

This code is exercised by an upcoming TFE provider change. It can be used to make the tfe_agent_pool data source much more efficient in organizations with more than 20 agents

External links

Output from tests

❯ go test .  -run TestAgentPoolsList -tags=integration
ok      github.com/hashicorp/go-tfe     3.740s

@JarrettSpiker JarrettSpiker requested a review from a team May 31, 2022
Copy link
Contributor

@sebasslash sebasslash left a comment

Nice and easy 🔥 ! Just missing the code documentation for the field.

agent_pool.go Show resolved Hide resolved
Copy link
Contributor

@sebasslash sebasslash left a comment

Really minor ⬇️

CHANGELOG.md Outdated Show resolved Hide resolved
@JarrettSpiker JarrettSpiker force-pushed the jspiker/agent-pool-search branch 2 times, most recently from e7e5f9a to 827e3c4 Compare Jun 8, 2022
Copy link
Contributor

@sebasslash sebasslash left a comment

🔥 Nice

Copy link
Contributor

@sebasslash sebasslash left a comment

Missed something ⬇️ , the test is failing in CI which means the query param is not available in tflocal.

@@ -68,6 +68,21 @@ func TestAgentPoolsList(t *testing.T) {
assert.Nil(t, pools)
assert.EqualError(t, err, ErrInvalidOrg.Error())
})

t.Run("with query options", func(t *testing.T) {

Copy link
Contributor

@sebasslash sebasslash Jun 8, 2022

Choose a reason for hiding this comment

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

Ah, I believe you mentioned this query param was sitting behind a feature flag... in which case skipIfBeta(t) test helper will need to be added to this subtest otherwise the test will fail in CI... I don't think tflocal has the flag enabled?

Copy link
Contributor Author

@JarrettSpiker JarrettSpiker Jul 11, 2022

Choose a reason for hiding this comment

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

@sebasslash sorry, took a vacation and let this review sit for a bit. The flag is now on in production, and it looks like we are seeing the tests pass!

@JarrettSpiker JarrettSpiker force-pushed the jspiker/agent-pool-search branch 3 times, most recently from 67cfe84 to ed40f7a Compare Jul 6, 2022
@JarrettSpiker JarrettSpiker force-pushed the jspiker/agent-pool-search branch 2 times, most recently from 074922e to adbb021 Compare Jul 11, 2022
@JarrettSpiker JarrettSpiker force-pushed the jspiker/agent-pool-search branch from adbb021 to 62d1a46 Compare Jul 12, 2022
Copy link
Contributor

@mpminardi mpminardi left a comment

This LGTM once the tests are passing (the one failure looks like a flake).

Copy link
Contributor

@sebasslash sebasslash left a comment

🔥 Looks good to me

@sebasslash sebasslash merged commit 833ec49 into main Jul 13, 2022
6 checks passed
@sebasslash sebasslash deleted the jspiker/agent-pool-search branch Jul 13, 2022
@github-actions
Copy link

github-actions bot commented Jul 13, 2022

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants