onelogin / terraform-provider-onelogin

GNU General Public License v3.0
27 stars 19 forks source link

inconsistent resource naming - singular v plural #56

Closed rjhornsby closed 1 year ago

rjhornsby commented 3 years ago

There's a bit of inconsistency in if the resource name is singular onelogin_role or plural onelogin_roles.

The actual resource names appear to be the plural form. That is, in TF code we use onelogin_roles. However, the TOC on the left side refers to the resource in the singular, as do references in the test cases, ie:

base := GetFixture("onelogin_role_example.tf", t)
update := GetFixture("onelogin_role_updated_example.tf", t)

Glancing through the code, this applies to other OL resource types like oidc_apps as well.

Most likely someone started writing it one way and changed their mind later. I do that more than I care to admit. The singular resource form follows the convention of a (quick and non-scientific) sample of other providers' resources. IMHO, in most cases the singular form makes a bit more sense and reads clearer. In a given TF resource block, we're most often managing a single object, rather than many at once.

There are two different data sources, one singular data_source_onelogin_user and one plural data_source_onelogin_users. This makes complete sense - they're doing different things and returning different data types.

dcaponi commented 3 years ago

Thanks for the input. I'll be honest and say this is a lack of discipline on my part. If you're suggesting that everything be singular (e.g. onelogin_saml_app or onelogin_user) that's a mechanically easy change. I think this change will come out later than other suggested changes since a naming change may cause unexpected problems with users who are grandfathered into the plural naming convention.

I'll update once the changes are made.

rjhornsby commented 3 years ago

Totally with you. I find myself similarly lacking discipline - whether it's singular/plural or - / _

AFAIK there's no hard and fast rule, but it looks like the general Terraform convention is singular for the name of the resource. This can come in a later set of changes, and might even have a little while where using the plural generates a deprecation warning, but both styles work.