civo / terraform-provider-civo

Terraform Civo provider
https://www.civo.com
Mozilla Public License 2.0
71 stars 56 forks source link

[BUG] Make NetworkID and FirewallID Mandatory for Instance resource #351

Open uzaxirr opened 1 week ago

uzaxirr commented 1 week ago

Description

context: https://civo-community.slack.com/archives/C0186KTRLPP/p1728269229482269?thread_ts=1728189777.364919&cid=C0186KTRLPP

Acceptance Criteria

dipu989 commented 1 day ago

hello @uzaxirr @alessandroargentieri I would like to pick this up.

I went through the codebase of terraform-provider-civo. Here's my understanding of this issue and how I am approaching this problem.

My Understanding -

I see in the resource_instance.go file, network_id is marked as Optional.

image

While firewall_id is marked as a required field. In case it is not set, the default firewall will be used. (As per description)

image

Then, I check the resourceInstanceCreate() method, in case network_id is not passed, we fetch the default network and set the networkID.

image

While in case of firewall_id, we just check if it's present, we set the instance firewall.

image

My proposed solution and doubts -

1) Make the network_id as Required : true instead of Optional : true 2) Doubt - We are checking for the firewall_id after createInstance(config) call is made (See the below attached screenshot) So should we perform this check before calling the createInstance() method? Will that help? And in case firewall_id is not set, do we return an error as per the issue description?

image

Another question - If we were already setting default values for both firewall_id and network_id when they weren't passed, why did this issue occur in the first place?

Sorry for these many doubts, want to have full context for my understanding.