aliksend / terraform-provider-dokku

Mozilla Public License 2.0
7 stars 1 forks source link

Add alias option into various services and update helper #33

Closed gelleson closed 5 months ago

gelleson commented 5 months ago

This commit introduces an alias option into multiple services including Elasticsearch, MongoDB, Clickhouse, and many others. The alias option is optional, validated for an uppercase string, and helper function has been updated to facilitate this change. Furthermore, logic around creating service links have also been modified to account for the possible presence of an alias.

aliksend commented 5 months ago

Thank you for your participating. I think DoubleDashArg will be better name for your helper function. Tell me when PR will be ready to merge

gelleson commented 5 months ago

Thank you for your participating. I think DoubleDashArg will be better name for your helper function. Tell me when PR will be ready to merge

Sure will rename. But could take a look to import part I am not sure.

gelleson commented 5 months ago

@aliksend done renamed :)

aliksend commented 5 months ago

Also please consider to change alias validation regexp to ^[A-Z_]+$ instead of ...*. To not accept empty strings as valid aliases.

aliksend commented 5 months ago

take a look to import part I am not sure

What import part do you mean?

gelleson commented 5 months ago

Like this parts https://github.com/aliksend/terraform-provider-dokku/pull/33/files#diff-9df6ba92293b097139a7a6c96f0ccd4a8983bdbd88b350a50af3817ec723e054R229-R231

gelleson commented 5 months ago

@aliksend changed regex

gelleson commented 5 months ago

@aliksend when will finish review please could to merge it and release a tag. I will use it later :)

aliksend commented 5 months ago

You need read current value of alias in Read function and set it to state. Then you can revert modifications of ImportState function. Can you do it?

gelleson commented 5 months ago

@aliksend please could leave some example of some resource link then I will fix based on your example

gelleson commented 5 months ago

@aliksend is it possible?)

aliksend commented 5 months ago

No, another plan. Please use

                PlanModifiers: []planmodifier.String{
                    stringplanmodifier.RequiresReplace(),
                },

to always re-create resource when alias is modified.

Else you need to implement Update logic. You can see example of complex Update here - app_name can't be updated (you can see corresponding PlanModifier in declaration) but other fields can. I don't think it is a good solution in this case.

Using proposed approach you don't need to read alias because it cannot be updated through modifying terraform configuration.

gelleson commented 5 months ago

@aliksend as I remember alias update is not possible to update

aliksend commented 5 months ago

alias is not possible to update

So it is better to mark it as non-updatable for terraform (using PlanModifier RequiresReplace) to rule terraform to re-create resource instead of updating it.

gelleson commented 5 months ago

@aliksend done updated

aliksend commented 5 months ago

Do you think PR is ready to merge?

gelleson commented 5 months ago

@aliksend give few minutes to test on local env and then we can merge it. Is it possible to wait few minutes?

aliksend commented 5 months ago

Yep. Tell me when it will be ready and I will merge and make a new version

gelleson commented 5 months ago

@aliksend I have to go out. Will do it tests later tomorrow or after tomorrow

gelleson commented 5 months ago

@aliksend tested on locally. I've done some fixes

gelleson commented 5 months ago

Let's merge it.