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 cloud region resource #535

Merged
merged 6 commits into from
Apr 15, 2024
Merged

Add cloud region resource #535

merged 6 commits into from
Apr 15, 2024

Conversation

bobbyiliev
Copy link
Contributor

Closes #432

@bobbyiliev bobbyiliev requested a review from a team as a code owner April 3, 2024 18:58
@bobbyiliev bobbyiliev requested review from SangJunBak and removed request for a team April 3, 2024 18:58
@bobbyiliev bobbyiliev requested a review from RobinClowers April 15, 2024 09:11
@@ -0,0 +1,3 @@
resource "materialize_region" "example" {
region_id = "aws/eu-west-2"

Choose a reason for hiding this comment

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

Seems weird to have a region that doesn't exist in our example?


req.Header.Add("Content-Type", "application/json")

resp, err := c.FronteggClient.HTTPClient.Do(req)

Choose a reason for hiding this comment

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

Why does this use the frontegg client? This is the cloud region api, right?

Copy link
Contributor Author

@bobbyiliev bobbyiliev Apr 15, 2024

Choose a reason for hiding this comment

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

Hmm, I think that this has been flawed from the initial implementation during the large refactor a few months ago.

As far as I can tell, the main idea seems to have been to reuse the FronteggClient's HTTPClient which already includes the Authorization token.

I'll have to refactor this. I've created an issue to track this and will submit a PR tomorrow! #543

Thanks for pointing it out!

@@ -115,6 +116,39 @@ func (c *CloudAPIClient) GetRegionDetails(ctx context.Context, provider CloudPro
return &region, nil
}

// EnableRegion sends a PATCH request to enable a region and polls until the region is enabled or a timeout is reached.

Choose a reason for hiding this comment

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

It seems like this comment describes the region resource, this function doesn't appear to do any polling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I initially had it setup that way, but a better approach is to do the polling in the resource itself rather than the client lib, so I moved the waitForRegionToBeEnabled to the resource itself. Will update that comment accordingly!

@bobbyiliev bobbyiliev merged commit 43990b5 into main Apr 15, 2024
6 checks passed
@bobbyiliev bobbyiliev deleted the Add-Cloud-Region-Resource branch April 15, 2024 18:22
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.

Resource materialize_region
2 participants