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 firehydrant_teams and firehydrant_team data source #121

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

splittingred
Copy link
Contributor

@splittingred splittingred commented Apr 20, 2023

Description

Adds two new data sources, as per #108

  • firehydrant_teams - with query as a filter
  • firehydrant_team - with id argument

Also aligns teams interfaces with sub-struct approach of other areas.

Also, had to update the go.mod to 1.18 (the test suite was already on 1.18(!)), as a lot of the deps were no long er 1.16-compatible.

Testing plan

  • Unit tests should pass

Related links

PR readiness

  • Relevant documentation has been updated if this PR adds or updates attributes, resources, or data sources.
  • An entry has been added to the changelog, if necessary.

CreateTeam(ctx context.Context, req CreateTeamRequest) (*TeamResponse, error)
UpdateTeam(ctx context.Context, id string, req UpdateTeamRequest) (*TeamResponse, error)
DeleteTeam(ctx context.Context, id string) error
Teams() TeamsClient
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks so much for moving this into a proper interface. 👏

provider/teams_data.go Show resolved Hide resolved
@splittingred
Copy link
Contributor Author

Hi @bobbytables - thanks for the review! Any chance you or someone from your team could take another look? Thanks!

@splittingred
Copy link
Contributor Author

@bobbytables Any help here? Would be great to get this in, or something similar. It's blocking our ability to utilize firehydrant via terraform. Is there anything we can do to help?

Copy link
Contributor

@elg0nz elg0nz left a comment

Choose a reason for hiding this comment

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

This PR looks quite solid. And to be frank upgrading go version should be relatively safe, but I would still like to see it on a different PR just in case there are any regressions :)

go.mod Outdated
@@ -1,22 +1,64 @@
module github.com/firehydrant/terraform-provider-firehydrant

go 1.16
go 1.18
Copy link
Contributor

Choose a reason for hiding this comment

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

@splittingred is it necessary to bump the go version for this PR?
Would it be possible to open a separate PR with just the version change?

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elg0nz yep, I can. This provider is currently broken on main as:

# github.com/zclconf/go-cty/cty/set
note: module requires Go 1.18
# github.com/hashicorp/terraform-plugin-go/internal/logging
note: module requires Go 1.18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,44 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice docs

@sofuture
Copy link
Contributor

@splittingred if you don't mind rebasing, this looks good to me for merge and release.

@splittingred
Copy link
Contributor Author

@sofuture Done, though I believe this still needs #127 (or #125 ) to merge first, no?

firehydrant/teams_test.go Outdated Show resolved Hide resolved
firehydrant/teams_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sofuture sofuture left a comment

Choose a reason for hiding this comment

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

last one hopefully!

firehydrant/teams_test.go Outdated Show resolved Hide resolved
@sofuture sofuture merged commit 2a5d38d into firehydrant:main Jul 31, 2023
1 check failed
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.

4 participants