Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exception for kubernetes_cluster_role's non_resource_urls rule field #3

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

pdecat
Copy link
Contributor

@pdecat pdecat commented Apr 2, 2019

This PR adds an exception for the conversion of kubernetes' nonResourceURLs ClusterRole & Role rule field into the terraform provider's non_resource_urls casing.

Without this change, the field is converted as non_resource_ur_ls.

I believe the underlying issue is that converting plain strings poses an issue with plural acronyms for which case is not explicitly defined:

Go field names must be CamelCase. JSON field names must be camelCase. Other than capitalization of the initial letter, the two should almost always match. No underscores nor dashes in either.
[...]
Acronyms should similarly only be used when extremely commonly known. All letters in the acronym should have the same case, using the appropriate case for the situation. For example, at the beginning of a field name, the acronym should be all lowercase, such as "httpGet". Where used as a constant, all letters should be uppercase, such as "TCP" or "UDP".

cf. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#naming-conventions

I believe the conversion process should take the source casing into account (e.g. from Camel Case to Snake Case), or better yet have some pivot format defining at the very least what's a word, an acronym, and if it's plural or singular. I did not find any Go implementation doing that yet.

@sl1pm4t sl1pm4t merged commit 6ab34b7 into sl1pm4t:master Apr 3, 2019
@sl1pm4t
Copy link
Owner

sl1pm4t commented Apr 3, 2019

Thanks @pdecat. The iancoleman/strcase library does having some logic to handle acronyms, but I guess not pluralised acronyms.

@pdecat pdecat deleted the add-non_resource_urls-exception branch April 3, 2019 10:35
@pdecat
Copy link
Contributor Author

pdecat commented Apr 3, 2019

Thanks!

The iancoleman/strcase library does having some logic to handle acronyms, but I guess not pluralised acronyms.

I initially planned to submit a PR to that library then found out it would probably be way too intrusive to get accepted. I glanced at https://github.com/stoewer/go-strcase too, and it looked the same.

@pdecat
Copy link
Contributor Author

pdecat commented Apr 3, 2019

Sorry, this PR lacked real world testing and the role.rule. path used is incorrect.
Fix PR: #6 .

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