Skip to content

Conversation

@k-yomo
Copy link
Contributor

@k-yomo k-yomo commented Dec 18, 2022

#214

This PR adds provider for terraform-plugin-framework.

TODO

@k-yomo k-yomo force-pushed the add-framework-provider branch 6 times, most recently from 683e8e4 to 5b8fd70 Compare December 18, 2022 16:01
@k-yomo k-yomo force-pushed the add-framework-provider branch from 5b8fd70 to bce48bc Compare December 18, 2022 16:08
@tobio
Copy link
Member

tobio commented Dec 19, 2022

This will conflict a bunch with #226. @k-yomo if you want to keep working on this one right now that's fine, but it might be easier if we hold off here for a couple of weeks until #226 is merged.

@k-yomo
Copy link
Contributor Author

k-yomo commented Dec 19, 2022

@tobio
Oh I see. Thank you for the heads-up!
I'll work on this after #226 is done!

return nil, diags
}

func NewFWEsApiClient(ctx context.Context, esConn *ElasticSearchConnection, version string, useEnvAsDefault bool) (*ApiClient, fwdiag.Diagnostics) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func NewFWEsApiClient(ctx context.Context, esConn *ElasticSearchConnection, version string, useEnvAsDefault bool) (*ApiClient, fwdiag.Diagnostics) {
func NewFWApiClient(ctx context.Context, esConn *ElasticSearchConnection, version string, useEnvAsDefault bool) (*ApiClient, fwdiag.Diagnostics) {

We're working on adding Kibana support here too

config.Addresses = addrs

envInsecure, _ := strconv.ParseBool(os.Getenv("ELASTICSEARCH_INSECURE"))
if esConn.Insecure.ValueBool() || envInsecure {
Copy link
Member

Choose a reason for hiding this comment

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

So this is inconsistent with the other settings, where the environment value is checked even when the value is specified in the provider configuration. I think we should be consistent here.


const esConnectionKey string = "elasticsearch_connection"

type ElasticSearchConnection struct {
Copy link
Member

Choose a reason for hiding this comment

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

IMO it would be cleaner to have all functions which build a new client parse the relevant config into this struct and then call a single function which builds an elasticsearch.Config{} instance from this struct.

It would mean consolidating all the CA, TLS and debugging code. Given the different diagnostic types we'd have to return a plain error but I think that's ok.

WDYT?


const esConnectionKey string = "elasticsearch_connection"

type ElasticSearchConnection struct {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
type ElasticSearchConnection struct {
type ElasticsearchConnection struct {

Optional: true,
Sensitive: true,
Validators: []validator.String{
stringvalidator.ConflictsWith(path.MatchRoot(usernamePath)),
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead use a relative path matcher in the new schema?

Suggested change
stringvalidator.ConflictsWith(path.MatchRoot(usernamePath)),
stringvalidator.ConflictsWith(path.MatchRelative().AtParent().AtName("username")),

go.mod Outdated
google.golang.org/protobuf v1.28.1 // indirect
)

replace github.com/hashicorp/terraform-plugin-mux v0.7.0 => github.com/k-yomo/terraform-plugin-mux v0.0.0-20221218141521-7a01a5a64e24
Copy link
Member

Choose a reason for hiding this comment

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

Just flagging it so it doesn't get merged like this. Moving the maxitems validation to code in the short term is ok if getting your PR merged into terraform-plugin-mux turns out to be too slow.

Copy link
Contributor Author

@k-yomo k-yomo Jan 6, 2023

Choose a reason for hiding this comment

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

The below update contains the fix!
#235

@k-yomo
Copy link
Contributor Author

k-yomo commented Mar 24, 2023

Sorry for no updates here for a while, I'll restart working on this PR🙏

@tobio tobio closed this Dec 12, 2023
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