Rename attributes expected guids with _id suffix to match community conventions

[gberche-orange]

As suggested in #155 by @adamzerella, our attributes expecting guids do not currently follow community naming conventions and are lacking the "_id" suffix, and can be confusing with attributes that expect names instead of guids.

In reference tf resources such as https://www.terraform.io/docs/providers/aws/r/instance.html attributes expecting ids are suffixed with _id, such as primary_network_interface_id or subnet_id or vpc_id

For example,

resource "cloudfoundry_org" "my_new_org" {
  name = "demo"
}

resource "cloudfoundry_space" "demo-basel" {
  name = "demo-basel"
  org = "${cloudfoundry_org.my_new_org.id}"
}

should instead become:

resource "cloudfoundry_org" "my_new_org" {
  name = "demo"
}

resource "cloudfoundry_space" "demo-basel" {
  name = "demo-basel"
  org_id = "${cloudfoundry_org.my_new_org.id}"
}

Given the current state of mevansam/terraform-provider-cf and the fact that we're close to releasing 1.0 which comes with backward compatibility guarantees, we indeed need to decide whether to make this late change before hitting 1.0, as we won't be able to make it in the future.

[gberche-orange]

Assigning to 1.0 milestone as to bring visibility to the team, and trigger the discussion soon.

[gberche-orange]

Benefits:

• be consistent with TF provider conventions
• be easier/more intuitive to use
• allow to future attribute accepting resource names instead of ids

Impacts:

• if made as an incompatible change, needs to be done before 1.0 as to avoid the community to have to perform migrations in the future
• work for the team to perform a bulk attribute rename in code as well as website documentation, applying https://www.terraform.io/docs/extend/best-practices/deprecations.html#renaming-a-required-attribute
• potential conflics from pending branches not yet merged from @jcarrothers-sap
• requires users to migrate from 0.9.9 to 0.10.0 in tf specs resource definitions, possibly using sed scripts

Example of TF specs change

resource "cloudfoundry_service_instance" "my_mysql" {
  name = "my_mysql"
  service_plan = "${data.cloudfoundry_service.elephantsql.service_plans["turtle"]}"
  space = "${cloudfoundry_space.demo-basel.id}"
}

becomes:

resource "cloudfoundry_service_instance" "my_mysql" {
  name = "my_mysql"
  service_plan_id = "${data.cloudfoundry_service.elephantsql.service_plans["turtle"]}"
  space_id = "${cloudfoundry_space.demo-basel.id}"
}

[gberche-orange]

@lixilin2301 is it something you'd be interested in investigating and preparing ?

[lixilin2301]

I'll work on it. I think we'll need some time for this, since it also impacts docs and test code.

Please update me if @jcarrothers-sap wants to merge before this change, so we could avoid some conflicts.

[janosbinder]

We would also need to cover this by the migration script (https://github.com/mevansam/terraform-provider-cf/pull/165). Otherwise it will be a lot of manual mangling, when we implement this.

[gberche-orange]

Discussions in today's weekly meeting:

• should this change introduce a backward incompatible change, or instead deprecate old attributes without the _id suffix, and introduce new ones ?
  • given the large number of attributes involved, the fact that old attributes without the _id suffix would eventually be removed, that other incompatible changes were introduced in 0.9.9 while renaming the provider with helper scripts to ease renamings (#165), the team has a preference to keep things simple in the implementation and rename the attribute.
• get confirmation of community convention for string attributes expecting guids, and good practices of suffixing them with _id

[gberche-orange]

Discussions in today's weekly meeting with @lixilin2301

• introducing such change prior to merging other PRs is likely to create lot of work related to git merge conflicts in other pending PRs, mostly from @jcarrothers-sap
• next step is then to make a short spike to study whether preserving backward compatibility would be too complex/costly in the future.
  • If backward compatibility is affordable, it is acceptable to introduce even with a larger user base (ie after terraform-providers/ graduation)
  • If backward compatibility would be too complex to maintain, then we should prioritize this early on before user base growth

@ArthurHlt and @psycofdj is it something you could contribute to ?

Again, feedback on this proposal is always welcome.

[psycofdj]

FRom my point of view, backward compatibility is affordable at small cost:

1. change name of all id type attribute from X to X_id in all resources definition
2. create a function taking a resource as input, that walks the schema and automatically an old X derpecated attribute for each X_id found
3. call the function on all resources in the provider definition
4. create .Set and .Get functions that abstract the resource implementation from the _id suffix and use them in resource implementation

This is probably something I can work on by the end of January 2019 (hopefully end of December)

[lixilin2301]

@gberche-orange Create a branch 'rename_id_suffix' to rename the resources, as @psycofdj said, 1 is done I think, which now passes acceptance tests. 234 are still left to do and I'm thinking of how to do it. In addition, I think some minor changes left in data_source need to adapted.

[gberche-orange]

@jcarrothers-sap please beware that your pending PRs are likely to conflict with https://github.com/mevansam/terraform-provider-cf/tree/rename_id_suffix

[gberche-orange]

Moving to post 1.0 since we have an outline to bring this without breaking change in the future