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 option to limit output id length #98

Merged
merged 9 commits into from
Aug 26, 2020
Merged

Conversation

tibbing
Copy link
Contributor

@tibbing tibbing commented Aug 11, 2020

what

  • Adds a new optional variable id_max_length, that when set to a value greater than 0 will limit the length of ID output to given value.
  • The truncated ID is still disambiguated by suffixing the first 5 characters of the md5 hash of the full ID.
  • The full ID will still be available as a new output id_fulll.
  • The default value of id_max_length is 0, so that the change is not breaking any existing usages.

why

  • Many resources have a max allowed length of names, such as aws_lb and aws_alb_target_group (32 characters), aws_iam_role (64 characters). Setting the id_max_length variable ensures that the max length is never exceeded, no matter the number of included attributes.

references

@osterman osterman requested review from aknysh and Nuru August 14, 2020 09:02
main.tf Outdated
@@ -56,7 +56,14 @@ locals {

labels = [for l in local.label_order : local.id_context[l] if length(local.id_context[l]) > 0]

id = lower(join(local.delimiter, local.labels))
full_id = lower(join(local.delimiter, local.labels))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use id_full to be consistent with the other ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or id_original

@Nuru @aknysh any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osterman Updated 👍

By review suggestion
@Nuru
Copy link
Contributor

Nuru commented Aug 26, 2020

@osterman Sorry to disagree with you, but since this is a new feature, anyone who wants to use it will have to make changes. Therefore, I think we should retain id as full-length ID and instead add 2 new fields, something like

  • limited_id_length_limit
  • limited_id

@aknysh
Copy link
Member

aknysh commented Aug 26, 2020

agree with @Nuru , no need to change the original id output since it's in use in hundreds of modules by thousands of people.
Let's make the new vars as @Nuru suggested - people who need the limited ID will just use the new output

@tibbing
Copy link
Contributor Author

tibbing commented Aug 26, 2020

@osterman Sorry to disagree with you, but since this is a new feature, anyone who wants to use it will have to make changes. Therefore, I think we should retain id as full-length ID and instead add 2 new fields, something like

  • limited_id_length_limit
  • limited_id

@Nuru @aknysh No, this PR has no breaking behavior or require any changes in existing usages. The id field will always represent the full ID unless max_id_length has been specified.

@aknysh
Copy link
Member

aknysh commented Aug 26, 2020

@osterman Sorry to disagree with you, but since this is a new feature, anyone who wants to use it will have to make changes. Therefore, I think we should retain id as full-length ID and instead add 2 new fields, something like

  • limited_id_length_limit
  • limited_id

@Nuru @aknysh No, this PR has no breaking behavior or require any changes in existing usages. The id field will always represent the full ID unless max_id_length has been specified.

Then no need to change the original ID at all, and no need to change its logic because many people are using it now.
Let's create another output with all the truncation logic you are proposing.

@aknysh
Copy link
Member

aknysh commented Aug 26, 2020

I understand why would you want to be able to modify the original ID - to keep the caller code the same (using just the id).
We will have two ID outputs in any case:

  1. The original unmodified ID and a truncated ID

or

  1. The original truncated ID and the new unmodified ID

@Nuru @osterman which way would be better?

@tibbing
Copy link
Contributor Author

tibbing commented Aug 26, 2020

I understand why would you want to be able to modify the original ID - to keep the caller code the same (using just the id).
We will have two ID outputs in any case:

  1. The original unmodified ID and a truncated ID

or

  1. The original truncated ID and the new unmodified ID

@Nuru @osterman which way would be better?

Yes exactly, in my mind it was more intuitive to not have to use a different output just because you have set a length limit. If you do set the max_id_length, you would probably in most cases never actually need the full ID, but it's there as a second output just in case.

@osterman
Copy link
Member

Ya the benefit with @tibbing approach is that it works seamlessly provided we add the max len input. Also since it defaults to zero, it has no effect unless the user overrides it. I like supporting the explicit outputs as well, but that doesn't work unless a module implements support for choosing the id. I think setting the max len should be used sparingly by end users as needed and this gives users an escape hatch when needed, that our modules currently do not provide.

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@Nuru
Copy link
Contributor

Nuru commented Aug 26, 2020

/terraform-fmt

@Nuru
Copy link
Contributor

Nuru commented Aug 26, 2020

/rebuild-readme

@Nuru Nuru self-requested a review August 26, 2020 22:37
@Nuru
Copy link
Contributor

Nuru commented Aug 26, 2020

/test all

@Nuru Nuru merged commit af6fd1f into cloudposse:master Aug 26, 2020
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.

Ability to set max length of ID output
5 participants