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

Create Watchtower Project Resources #1

Merged
merged 14 commits into from
May 29, 2020
Merged

Conversation

talanknight
Copy link
Contributor

This PR adds watchtower provider, and the project resource.

Acceptance tests will be added later as I figure out the best way to spin up a watchtower instance in association with the test.

@talanknight talanknight requested a review from jefferai May 18, 2020 18:49
@talanknight talanknight requested a review from malnick May 21, 2020 16:56
@talanknight talanknight marked this pull request as ready for review May 26, 2020 20:54
},
ResourcesMap: map[string]*schema.Resource{
"scaffolding_resource": resourceScaffolding(),
"watchtower_project": resourceProject(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the provider is called watchtower I vote to have the key here be just project so it doesn't duplicate the primary namespace of the provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to make the change. I thought that since different providers can be included in a single tf config the resources needed to be named in a way that won't collide with resources from other providers. For example, all github related resources have a "github_" prefix and the google related resources have "google_" prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, I agree with your assessment. This makes sense as-is, let's leave it.

Copy link
Contributor

@malnick malnick left a comment

Choose a reason for hiding this comment

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

Had a couple of generic comments but otherwise, this looks great! One interesting train of thought arose for me around org resource management and global provider configuration, but it's more of a musing right now than actual feedback. 🚀

client.SetOrg(d.Get("default_organization").(string))

// TODO: Pass these in through the config, add token, etc...
client.SetLimiter(5, 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to expose the SetLimiter values in the provider schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe at first? I think in the long run we may want to set a reasonable initial default and then allow the controller and go client to handle backoffs and retries. Thoughts?

Update: resourceProjectUpdate,
Delete: resourceProjectDelete,

// TODO: Add the ability to define a parent org instead of using one defined in the provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment surfaced some interesting thinking about architecture here: do we want resources within an instance of this provider to be able to provision resources within another org not declared outside the provider instance?

If we do allow resources to be created within orgs that are not globally defined to the instance of the provider, we'll have to bake in org logic into every resource within the provider. There was a similar issue with Vault's provider and Namespace support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this TODO and changed the provider's schema key from "organization" to "default_organization" after a comment from @jefferai where he seemed to hint at the need to do multi org config in a single tf config. The go client does facilitate setting a default org for the whole client and allowing a per call override. I think that would just mean that the resource logic would be checking to see for an org defined at the resource level and if so, using that for the calls through the go client.

@talanknight talanknight merged commit 5bb8c82 into master May 29, 2020
@talanknight talanknight deleted the resources-projects branch June 9, 2020 18:25
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.

2 participants