-
Notifications
You must be signed in to change notification settings - Fork 103
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 agent data source #456
Conversation
113e384
to
2c1cb2b
Compare
35977f5
to
84413bc
Compare
84413bc
to
cd5c837
Compare
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 items for discussion down below ⬇️ . Great work so far 👍
} | ||
|
||
u := fmt.Sprintf("agents/%s", url.QueryEscape(agentID)) | ||
req, err := s.client.NewRequest("GET", u, nil) |
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.
I think we should pass options
here instead of nil
.
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.
Possibly another sign that this method isn't needed! 😄
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.
@sebasslash 👍
@nfagerlund Can you elaborate on why it's a sign that the method, possibly, isn't needed❔
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.
Impressive work! I think I spotted some stuff that isn't right, plus a few things that might not be intended.
} | ||
|
||
u := fmt.Sprintf("agents/%s", url.QueryEscape(agentID)) | ||
req, err := s.client.NewRequest("GET", u, nil) |
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.
Possibly another sign that this method isn't needed! 😄
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.
Be sure to add this file to the mocks script and then generate mocks
Tests passing 🎉 |
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.
I made the change that I recommended and smoke tested it and it worked well. So close!
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.
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. |
Description
This PR adds a new datasource to the Client. The purpose is to allow users to Read, List agents. Please see the Agent Pool and Agents documentation, linked below, for more information.
Please be aware that this PR downloads the tfc-agent binary, which only supports Linux AMD64 architecture.
Testing plan
-- or --
External links
Output from tests
PASS
ok github.com/hashicorp/go-tfe
See example of how to run integration tests, below. Refer to TESTS.md for further details.