InfluxCommunity / influxdb3-go

The go package that provides a simple and convenient way to interact with InfluxDB 3.
https://pkg.go.dev/github.com/InfluxCommunity/influxdb3-go
MIT License
21 stars 11 forks source link

feat: add serverless bucket creation support #86

Closed xorphitus closed 4 weeks ago

xorphitus commented 1 month ago

Proposed Changes

Support bucket creation for InfluxDB Serverless.

NOTE: The API is incompatible with Cloud Dedicated database creation API. Therefore, probably Cloud Dedicated database creation should be implemented as another function, e.g., CreateDatabase.

Checklist

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.16%. Comparing base (f95a9ff) to head (13c3dc3).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #86 +/- ## ========================================== - Coverage 83.34% 78.16% -5.19% ========================================== Files 12 13 +1 Lines 1009 1044 +35 ========================================== - Hits 841 816 -25 - Misses 138 199 +61 + Partials 30 29 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

powersj commented 1 month ago

Therefore, probably Cloud Dedicated database creation should be implemented as another function, e.g., CreateDatabase.

@bednar - I would prefer to see some sort of namespacing or separation in this API between the different products as the APIs are different. It could be really confusing for users if they have to figure out which function belongs to which product. Thoughts?

bednar commented 1 month ago

@powersj, You’re absolutely right. We need to consider how to incorporate this into our source base to be sure that it’s clear to users which product they are interacting with.

xorphitus commented 1 month ago

@powersj @bednar Thank you for your comments. Should I wait for your namespacing design or should I come up with it by myself?

bednar commented 1 month ago

@xorphitus, please feel free to create suggestions for namespacing. We welcome your input!

thulasirajkomminar commented 1 month ago

@xorphitus I have some working version of library for cloud dedicated let me know once you come up with the namespaces.

bednar commented 1 month ago

@xorphitus, what are your thoughts on using a separate management directory along with a serverless.go file? We could name the functions as ServerlessCreateBucket, ServerlessUpdateBucket, and ServerlessListBuckets. Does this structure seem logical to you?

xorphitus commented 1 month ago

@bednar

using a separate management directory along with a serverless.go file

Sounds clean, but I think management/serverless.go doesn't work, and an easy way is management_serverless.go with a flat directory structure. The bucket creation function that I implemented reuses influxdb3.Client's package private methods and fields, so they must belong to the same directory.

As for function names, I propose the following; struct for namespacing. What do you think about this?

// management_serverless.go
type ServerelessClient struct {
    client *Client
}

func NewServerlessClient(client *Client) *ServerelessClient {
    return &ServerelessClient{client: client}
}

func (c *ServerelessClient) CreateBucket(ctx context.Context, bucket *Bucket) error {
    // Delegates some operations to `c.Client`
}

// Caller code
client, err := influxdb3.New(...)

serverlessManager := NewServerlessClient(client)
serverlessManager.CreateBucket(...)
serverlessManager.UpdateBucket(...)
serverlessManager.ListBucket(...)

I considered it might be overkill, and prefixing that you suggested might be sufficient, but administration tasks have very different characters from data read/write operations, so it would be okay to make it a separated client struct.

The drawback is that users may have to have at most three clients, influxdb3.Client, Serveless manager, and Cloud Dedicated manager, in a case where they need to use both Serverless and CloudDedicated, e.g., for development and production environments. It may look awkward.

bednar commented 1 month ago

Your proposal looks great 👍. Let’s proceed as you suggested.

xorphitus commented 1 month ago

@powersj @bednar It's ready for code reviewing. I've applied the namespacing and fixed CI errors.

jamesbalcombe83 commented 4 weeks ago

@powersj @bednar I'm looking into why the codecov report shows a reduction in coverage (-5.19%) for @xorphitus. It looks like the code that is indicated as no longer being covered should be handled by client_e2e_test.go . I tried to run those test in my local, but couldn't.

Can you fill use in on what are we missing to be able to verify that these lines are in still covered? Or can you see any reason why those lines are no longer covered based on the content of the PR?

bednar commented 4 weeks ago

Hi @xorphitus,

The end-to-end tests do not run for pull requests from third-party repositories to prevent exposing sensitive credentials. This limitation will be addressed once InfluxDB 3 OSS is released.

I will begin reviewing your contribution as soon as possible.

Best regards

bednar commented 4 weeks ago

@jamesbalcombe83, please see the comment above for more details: https://github.com/InfluxCommunity/influxdb3-go/pull/86#issuecomment-2160121455

xorphitus commented 4 weeks ago

@bednar I made an example file with 57ab218967d648bc71fa6df9a6d812f7090adeaa. Is this sufficient?

I also applied one more commit 13c3dc32582de44381c3e3341ad36d2fd334e83d because I found I missed licence texts.