Allow multiple truncated versions of a label

[alexjurkiewicz]

Truncated forms of id_full which are always available. This is useful
when you want to use the same label for several resources with different
length restrictions.

Closes #117.

[alexjurkiewicz]

There's an alternate implementation I considered:

module "label" {
  id_lengths = [16, 32, 64, 128] # default value. Add or remove values as required.
}

module.label.id # unchanged: truncated to `id_length_limit`
module.label.id_trunc[16] # 16 char version
module.label.id_trunc[32] # 32 char version
module.label.id_trunc[48] # Error! You didn't request this length in `id_lengths`

It's more configurable, but more complex. Do you think this complexity is worth it?

[alexjurkiewicz]

How do I run the bats tests locally on macOS? It's not documented as far as I can see. Is it this bats or something else?

[Nuru]

See #117 (comment) which is along the lines of your #118 (comment)

[alexjurkiewicz]

My concern with this approach is it's not possible to statically detect errors like "label.id[47]" (when you meant 48). This would result in run-time errors not picked up during validation. Do you still prefer this approach?

…

On Wed, 6 Jan 2021 at 23:51, Nuru ***@***.***> wrote: ***@***.**** requested changes on this pull request. See #117 (comment) <#117 (comment)> which is along the lines of your #118 (comment) <#118 (comment)> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#118 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC4U5MLWXOONIHQ7TXH24DSYRMD3ANCNFSM4VXDT4AQ> .

[osterman]

For this particular use-case, I think I agree with @alexjurkiewicz. Having the ability for terraform to statically detect errors is advantageous. Additionally, we get the benefit of automatic documentation.

[danjbh]

For this particular use-case, I think I agree with @alexjurkiewicz. Having the ability for terraform to statically detect errors is advantageous. Additionally, we get the benefit of automatic documentation.

@osterman I concur 👍

[aknysh]

/rebuild-readme

[aknysh]

/test all

[aknysh]

@alexjurkiewicz LGTM, thanks.
The only thing, can you add the new outputs to the examples here https://github.com/cloudposse/terraform-null-label/tree/master/examples/complete ?
Maybe create another label, so we could see the outputs in the tests.
Thank you

[alexjurkiewicz]

@aknysh done:

label8_truncated_ids = {
  "id" = "cloudposse-uat-6099f"
  "id128" = "cloudposse-uat-build-winstonchurchroom-big-fat-honking-cluster-failover"
  "id16" = "cloudposse-6099f"
  "id32" = "cloudposse-uat-build-winst-6099f"
  "id64" = "cloudposse-uat-build-winstonchurchroom-big-fat-honking-clu-6099f"
  "id_full" = "cloudposse-uat-build-winstonchurchroom-big-fat-honking-cluster-failover"
}

[aknysh]

/test all

[aknysh]

@alexjurkiewicz LGTM.
@osterman @Nuru
Let's review one more (the idea and implementation) time since the label module is used everywhere and we don't want to make mistakes here

[alexjurkiewicz]

One thought I had was removing id128 because it doesn't sort nice in the docs, and really who cares about an id if it's longer than 64 chars anyway?

[aknysh]

One thought I had was removing id128 because it doesn't sort nice in the docs, and really who cares about an id if it's longer than 64 chars anyway?

Yes, that's what I meant by reviewing one more time :)

[osterman]

[osterman]

[osterman]

So I really like the idea of pre-defined lengths, but it seems like in true AWS fashion they have picked some odd length limits. Maybe we need to go with a more dynamic approach as @Nuru suggested.

[tgourley01]

I was in favor of the fixed size outputs but now wonder if using a simple map to lookup size limits for a given resource type is all that is needed. I really don't want to have to know the limits for each resource type but that is I have been doing.
id_length_limit = local.awsResourceNameLimits["ec2"]

[alexjurkiewicz]

This module is cloud-agnostic, I think. So I'm not sure putting all that data into this module is a good idea. I'm on holidays a few weeks, but I'll update this PR to use user defined lengths when I get back.

…

On Thu, 21 Jan 2021, 5:22 am Tim Gourley, ***@***.***> wrote: I was in favor of the fixed size outputs but now wonder if using a simple map to lookup size limits for a given resource type is all that is needed. I really don't want to have to know the limits for each resource type but that is I have been doing. id_length_limit = local.awsResourceNameLimits["ec2"] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#118 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC4U5JXJTEJ5MY7UJTKKEDS24NQBANCNFSM4VXDT4AQ> .

[tgourley01]

This module is cloud-agnostic, I think. So I'm not sure putting all that data into this module is a good idea. I'm on holidays a few weeks, but I'll update this PR to use user defined lengths when I get back.
[…]

I did not mean put the resource type to size mapping in this module. But saying - a convention of using a map to apply the correct size for a resource type would be handy, instead of attempting to hard code it per resource. (that is what I tried to imply at least with the local reference, but I wasn't clear enough.)

[Nuru]

@alexjurkiewicz @osterman @aknysh At this point I'm thinking this is better off published as a separate module: teraform-null-label-truncate or something like that, so that it can have a huge number of outputs without burdening every user of terraform-null-label with it. It could have a fixed-length output for every length from 8 to 128 using zero prefixes so that they sort numerically:

label_008
label_009
label_010
...

You would use it as an add-on the null-label:

module "label" {
  source  = "cloudposse/label/null"
  version = "0.22.1"

  attributes = ["example"]

  context = module.this.context
}

module "short_label" {
  source  = "cloudposse/label-truncate/null"
  version = "0.1.0"
 
  delimiter  = module.label.delimiter
  label_full = module.label.id_full
}

locals {
  elasticache_id    = module.short_label.label_050
  aurora_cluster_id = module.short_label.label_063
}

The difference between a straightforward chopping off of the string and what this module does is that the module will include a partial hash of the full string, so that the strings will be different even if the substrings are the same, and uses and respects the delimiter, like null-label currently does with the truncated id. Also, we can have an option about where to put the hash. Null Label always puts the hash at the end, but it might be better to put it in the middle or at or near the beginning.

We could even expand on null-label's label_order to include a "hash element, so you could have ["namespace", "hash", "environment", "stage", "name", "attributes"] which means if a hash is needed, put it after namespace and delete forward until the remaining string is short enough to include. Or to keep from having to re-include or duplicate features of null-label, we could just take an index of how many delimiters we should skip from beginning or end before inserting the hash.

So it can be complex and sophisticated but only used when needed, so that terraform-null-label can remain lighter and updates to it are not so disruptive the Cloud Posse Terraform module ecosystem.

[osterman]

better off published as a separate module

@Nuru probably not a separate module, but a submodule of this one. That submodule should probably only accept context and none of the other flags.

[Nuru]

better off published as a separate module

@osterman I'm not that familiar with publishing a submodule. What are the advantages of that? My thought about publishing it separately is that it is a handy utility anywhere you need to shorten strings while maintaining uniqueness.

@Nuru probably not a separate module, but a submodule of this one. That submodule should probably only accept context and none of the other flags.

context contains all the flags, so I'm not sure what you mean by "none of the other flags", but I think there are a few options it should take, like length of hash to use and some kind of option about where to place the hash. Maybe also whether the hash should be upper or lower case and/or all alphabetic or hexadecimal.

[alexjurkiewicz]

Rebased.

consider how we are now handling the hash and the letter case formatting

Luckily this merged cleanly, I manually checked and these changes look to be completely uncoupled.

Please also include changes from #120 and bump the version number in context.tf to 0.24.0

Done ✌️

[alexjurkiewicz]

One question. Should I pass the id_lengths variable as part of the output context? If so, how should I handle it when a context is passed in? Should I merge with the user-supplied variable or replace it entirely?

I lean towards: "yes, include it in context and a user-supplied override should replace".

[Nuru]

@alexjurkiewicz All inputs need to be carried forward to the output context to support chaining. For id_lengths, they should be merged. I think it will be exactly like attributes except for the text operations (regex replace, lower case).

Be sure to include tests where var.id_lengths and var.context.id_lengths are both supplied and verify you get the correct output.

[alexjurkiewicz]

All inputs need to be carried forward to the output context to support chaining. For id_lengths, they should be merged. I think it will be exactly like attributes except for the text operations (regex replace, lower case).

Done. Since this is a list of numbers rather than strings the logic is slightly different to attributes, also I added non-null variable validation for individual list elements (since Terraform numbers are nullable).

Be sure to include tests where var.id_lengths and var.context.id_lengths are both supplied and verify you get the correct output.

Done (see label9.tf)

[Nuru]

All inputs need to be carried forward to the output context to support chaining. For id_lengths, they should be merged. I think it will be exactly like attributes except for the text operations (regex replace, lower case).

Done. Since this is a list of numbers rather than strings the logic is slightly different to attributes, also I added non-null variable validation for individual list elements (since Terraform numbers are nullable).

The only difference between strings and numbers is that you do not have to worry about modifying the numbers. nulls should generally be OK and handled with compact or coalesce as appropriate. I do, however, want you to verify that the numbers are >=6 and <= 256. Although maybe @osterman disagrees.

Be sure to include tests where var.id_lengths and var.context.id_lengths are both supplied and verify you get the correct output.

Done (see label9.tf)

Sorry that it is not obvious or documented, but it is not enough to create the Terraform for the test, you have to validate the outputs in examples_complete_test.go. Also, please make sure that one of the truncation lengths lines up with a delimiter such that we get an extra character of hash, to verify that that is working. I think 21 will do it for label9.tf.

[Nuru]

@alexjurkiewicz I want to let you know that after spending 3 months and 23 commits to get #107 right (or so we thought), we then found a bunch of other problems leading to a failed v0.23.0 and v0.24.0, and now that we have v0.24.1 out we going to take a break on releasing new features. So we are probably not going to try to include the PR anytime soon. However, if you want to educate yourself, look at all the changes we made after merging #107 just to fix the things we overlooked after spending 3 months and 23 commits trying to get it right. It is a consequence of this being a heavily used module that needs to retain backward compatibility even as we add new features. The more of these lessons you can learn from and incorporate in this PR, the more likely we are to get this released sooner rather than later.

[alexjurkiewicz]

Thanks. Totally understand. So looks like you want to maintain forwards and backwards compatibility with new versions. Specifically: support passing old contexts into new version and vice versa. Is that right?

[Nuru]

So looks like you want to maintain forwards and backwards compatibility with new versions. Specifically: support passing old contexts into new version and vice versa. Is that right?

@alexjurkiewicz Yes, that is right. The plan we have is that going forward, the context input will be any, we will require all the fields in context from null-label v0.22.1 be present (that is, fail if they are missing), but any more fields we add will be optional and if missing, default to the default. Not how we did it for label_key_case, in particular how we preserve null values in the input. So we will want you to do the same for id_lengths while at the same time accumulating/combining id_lengths when they are non-null, similar to how we do with attributes.

You probably want to rebase this PR on master.

[alexjurkiewicz]

Thanks for the feedback. I have made the following changes:

• Rebased on master, which picks up a bunch of global improvements following label_case. Most importantly type = any for var.context.
• Added logic to support loading a context without id_lengths specified, so people can push a 0.24.0-era context into a newer version of this module without errors
• id_length is merged across multiple levels of context inheritance, accounting for duplicates as well
• test case 9 validates using id_lengths via variable, context, and both at the same time
• test case 9 is included in the Go integration test suite

I believe (once again :) ) this addresses everything. Please take a look 🙏

[alexjurkiewicz]

I've just added a test for loading an older context. It's the one thing I'm least sure about, since I couldn't base it on any prior art in this module ;) Please check b90a32a's content extra carefully.

[alexjurkiewicz]

ping

[Nuru]

@alexjurkiewicz I know you have worked hard on this, but I am still not convinced it is worth the extra overhead. I really hope id_length_limit is enough, and in the rare case it is not, you can always instantiate a new module with a new id_length_limit and feed it the same context.

@osterman What do you think?

If we were to go forward with this, I would want the id_lengths list to include id_length_limit automatically if it were set. So if id_length_limit = 44, id_trun would include "44" = "<id of <=44 characters>"

[Nuru]

@alexjurkiewicz I am going to close this and close #117 as "wontfix" because of all the reasons already stated, the main ones being:

• we have implemented a single id_length field so any resource that has a length limit can create a label that respects that limit by instantiating null-label and passing in the length limit and an existing context object
• we do not have evidence of a commonly used length that would be worth providing by default
• we do not have evidence this will provide a significant benefit to a significant number of people beyond what is provided by id_length (which had not been implemented when this PR was first opened)