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

Cloud Resources Refactor #351

Merged
merged 14 commits into from
Jan 10, 2024
Merged

Cloud Resources Refactor #351

merged 14 commits into from
Jan 10, 2024

Conversation

bobbyiliev
Copy link
Contributor

@bobbyiliev bobbyiliev commented Nov 8, 2023

I've been thinking with #5 based on the doc here

Each resource within our provider will be able to utilize one of these clients depending on its requirements. For example, a resource that manages database configurations will use the DBClient, whereas another that interacts with Frontegg services will use FronteggClient and etc.

struct {
	DB       *clients.DBClient
	Frontegg *clients.FronteggClient
}

This PR is more of a PoC so we could start a further discussion, it implements a materialize_app_password resource that utilizes the FrontEgg API:

resource "materialize_app_password" "example_password" {
  name = "tf-example-app-password2"
}

List of things that need to be done to get this PR merged:

  • Add a design doc
  • Add Frontegg Client
  • Add Tests for the Frontegg Client
  • Add Cloud API Client
  • Add Database Client
  • Add Cloud API Resources
  • Add Tests for the Cloud API Client
  • Refactor the Provider configuration to use the new clients
  • Refactor all existing database resources to use the client
  • Refactor all existing tests to use the client
  • Add the new Cloud Resources:
    • materialize_app_password
    • materialize_user
  • Add the new data sources:
    • materialize_regions

Refactor resources to use the new DB client:

  • Cluster Replicas
  • Clusters
  • Connection - AWS Privatelink
  • Connection - Confluent Schema Registry
  • Connection - Kafka
  • Connection - Postgres
  • Connection - SSH
  • Databases
  • Grants
  • Grants Default
  • Roles
  • Indexes
  • Materialized Views
  • Schemas
  • Secrets
  • Sinks - Kafka
  • Sources - Kafka
  • Sources - Load Generator
  • Sources - Postgres
  • Sources - Webhook
  • Tables
  • Types
  • Views

Tasks that can be contributed separately in follow-up PRs:

  • Add the new Cloud Resources:
    • materialize_region
  • Add the new data sources:
    • materialize_organization

@benesch
Copy link
Contributor

benesch commented Nov 9, 2023

Nice, this makes sense to me! Note that you probably want to support multiple "DB" clients, one per region:

type Region string

const (
        awsUsEast1 Region = "aws/us-east-1"
        awsUsWest2 Region = "aws/us-west-2"
)

struct {
	DB       map[Region]*clients.DBClient
	Frontegg *clients.FronteggClient
}

Copy link
Contributor

@dehume dehume left a comment

Choose a reason for hiding this comment

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

This looks great. Exciting to add these

@@ -60,8 +61,16 @@ func Provider() *schema.Provider {
DefaultFunc: schema.EnvDefaultFunc("MZ_SSLMODE", true),
Description: "For testing purposes, disable SSL.",
},
"endpoint": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the admin endpoint unique to each organization's Materialize account?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, but you'll need some way to override it for tests. Production and staging have different admin endpoints, for example.

@CLAassistant
Copy link

CLAassistant commented Nov 10, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Looking good! Dropped in a few comments.

docs/developer/design-v2-2023-11-11.md Outdated Show resolved Hide resolved
docs/developer/design-v2-2023-11-11.md Show resolved Hide resolved
docs/developer/design-v2-2023-11-11.md Outdated Show resolved Hide resolved
docs/developer/design-v2-2023-11-11.md Outdated Show resolved Hide resolved
docs/developer/design-v2-2023-11-11.md Outdated Show resolved Hide resolved
docs/developer/design-v2-2023-11-11.md Outdated Show resolved Hide resolved
docs/developer/design-v2-2023-11-11.md Show resolved Hide resolved
@bobbyiliev bobbyiliev force-pushed the cloud-resources branch 5 times, most recently from 0d858ad to 6b303bd Compare November 15, 2023 21:43
@bobbyiliev bobbyiliev changed the title WIP: cloud resources [WIP]: Cloud Resources Nov 16, 2023
@bobbyiliev bobbyiliev force-pushed the cloud-resources branch 6 times, most recently from 3a461ce to a6560dd Compare November 23, 2023 14:25
@bobbyiliev bobbyiliev added breaking change Will cause a breaking change in the next release new resource A new Terraform resource labels Nov 23, 2023
@bobbyiliev bobbyiliev force-pushed the cloud-resources branch 8 times, most recently from 8f0a202 to 53ff098 Compare November 24, 2023 12:58
@bobbyiliev bobbyiliev requested review from benesch and dehume November 24, 2023 13:27
Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Nice! I only took a very quick skim, but this all seems to be headed in a wonderful direction. 👍🏽

docs/developer/design-v2-2023-11-11.md Show resolved Hide resolved
Comment on lines 62 to 63
// Deep copy the request to ensure it's safe to modify
req2 := cloneRequest(req)
req2.Header.Set("Authorization", "Bearer "+t.Token)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you say more about why we need the deep copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is primarily related to concurrency and the immutability of the original request. As http.Request.Body can only be read once, a new body needs to be copied:

https://stackoverflow.com/questions/62017146/http-request-clone-is-not-deep-clone

Will try to adjust the comment to give a bit more details!

Copy link
Contributor

Choose a reason for hiding this comment

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

How about you include the stack overflow link in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, just added the link in the func comments.

@bobbyiliev bobbyiliev force-pushed the cloud-resources branch 3 times, most recently from ccce478 to 8fa2cc7 Compare January 8, 2024 08:55
@bobbyiliev bobbyiliev requested a review from sjwiesman January 8, 2024 12:55
bobbyiliev and others added 13 commits January 8, 2024 16:40
Terraform Docs

WIP: manage frontegg users

[WIP]: Cloud Resources

Terraform Docs

Add tests for app password

Terraform Docs

Fix failing tests

Add region datasource

Add integration tests for user resource

Minor changes

Terraform Docs

Minor changes

Add token refresh func
@bobbyiliev bobbyiliev merged commit c496aea into main Jan 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Will cause a breaking change in the next release new resource A new Terraform resource
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants