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

feat: add support for lowercased context tags #107

Merged
merged 23 commits into from
Feb 1, 2021

Conversation

SweetOps
Copy link
Contributor

@SweetOps SweetOps commented Nov 16, 2020

what

  • add support for use lowercased context tags

why

  • not all cloud-providers supports the uppercased keys for tagging/labeling resources

@SweetOps SweetOps requested a review from a team as a code owner November 16, 2020 13:34
@SweetOps
Copy link
Contributor Author

/test all

Copy link
Sponsor Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

I understand the need for the feature, but have some issues with this PR as is:

  1. Any new variable needs to be added:
    • as a variable with default null
    • as a member of the context object with default null
    • assigned a default value (if null) in main.tf in local.defaults and otherwise follow the pattern of precedence and pass through
    • be added to context.tf
  2. I do not like the name of the variable
  3. I do not like having a boolean feature toggle for something like this where there is likely to be an explosion of options.

Brainstorming for others (especially @osterman and @aknysh) to comment on:

How about a variable like tag_format that takes a string like "Title:none" to indicate that the tag name is transformed to Title case and the tag value is unchanged. We could then implement the current PR with tag_format = "lower:none" and provide some other options like "UPPER" and whatever else Terraform natively supports.

@Nuru Nuru added the wip Work in Progress: Not ready for final review or merge label Nov 16, 2020
@osterman
Copy link
Member

yes, mostly agree with @Nuru .

We're interested in the general functionality of this PR, but want to work with you get the interface right since the implications are far-reaching and involve all 130+ terraform modules we manage.

We'll want to future proof it so that we can support multiple "cases" (e.g. titlecase, lowercase, uppercase, snakecase, camelcase, kebabcase, etc). Also, a similar feature would be to add support for id_format (E.g. serverName instead of server-name) as well, so the ID could be encoded based on preferences.

So, I think something like what @Nuru proposes as the interface looks good.

Feel free to pin us in the #pr-reviews channel on slack for real-time discussion =)

@osterman osterman added the do not merge Do not merge this PR, doing so would cause problems label Nov 17, 2020
@aknysh
Copy link
Member

aknysh commented Nov 17, 2020

I agree we need to be able to do it generically for many outputs (id, tags`).
But do we need to transform the values of the tags? They are completely controlled by the user.

@Nuru
Copy link
Sponsor Contributor

Nuru commented Dec 2, 2020

@osterman I think we should start small and fill in the roadmap as demand arises. While this PR's feature is important for tags because currently we force Title Case on tag keys, generally I agree with @aknysh that the users have enough control on ID for now.

@SweetOps Thanks again for your contribution.

This is a lot further along, but not quite there. I think you misunderstood my proposal. The idea is that tag_format contains 2 components separated by the colon:

  • The first part is the formatting applied to the tag key
  • The last part is the formatting applied to the tag value

The default format would be "title:none". At a minimum, we would want to have formats

  • none
  • Title
  • UPPER
  • lower

(I want the formatting of the format name to reflect the format it selects, but do not want to do something crazy like nOne)

I think I would implement a sub-module that takes a map ($map_name) of maps ($key: $value) and returns a map ($map_name) of maps ($format) of maps (map_key, map_value) of maps ($key: $formatted) with each formatting option.

input:

{ 
  tags: {
    name: "null-label"
  }
}

output

{ 
  tags: {
    lower: {
      map_keys: {  name: "name" }
      map_values: { name: "null-label" }
    }
    Title: {
      map_keys: {  name: "Name" }
      map_values: { name: "Null-Label" }
    }
    UPPER: {
      map_keys: {  name: "NAME" }
      map_values: { name: "NULL-LABEL" }
   }
 }
}

@Nuru Nuru requested review from osterman and aknysh December 2, 2020 20:40
@osterman
Copy link
Member

osterman commented Dec 3, 2020

@Nuru we should not change the tags output because that will "break the internet". The tags output should be the same as it is today. Rather tags, should be:

output "tags" {
  value = local.tag_formats[var.tag_format]
}

@osterman
Copy link
Member

osterman commented Dec 3, 2020

aha, @Nuru you are suggesting the interface for the submodule. That makes sense.

@Sytten
Copy link

Sytten commented Dec 4, 2020

I was just about to open an issue for this, currently I can't use the tags on GCP because the keys must start with a lowercase letter. I hope this gets merged soon!

@imunhatep
Copy link

Hello, any news/plans regarding tag name formating?

@osterman osterman changed the title feat: add possability to use lowercased context tags feat: add support for lowercased context tags Jan 18, 2021
@Nuru
Copy link
Sponsor Contributor

Nuru commented Jan 18, 2021

Stepping back again, I think this is way overkill. The initial issue/request was that we allow lowercase tag names instead of forcing them to Title Case. I think we should do that (plus add a third option for UPPERCASE) and stop at that.

This current PR, as I understand it, uses a tag format specifier of valueFormat:nameFormat where valueFormat is one of:

  • default (which leaves values unchanged)
  • snake
  • kebab
  • pascal
  • camel

and nameFormat is either lowercase or titlecase. (I find it confusing that valueFormat comes first, since we generally put keys before values, but that is minor and easily changed.) More importantly, it leaves out UPPER and lower as options for valueFormat. And this only applies to tags, not the ID, which means that the name tag's value will not match the id in many cases, which I find problematic.

In the fully exploded feature, we would have an idFormat variable of the form format and a tagFormat to be nameFormat:valueFormat where any of the formats could be:

  • none (no change from input, generated names/attributes are lower case single words)
  • Title
  • Upper
  • lower
  • Pascal
  • camel
  • snake
  • kebab

But for tag values, this opens up the question of what constitutes a word boundary, since this module gets fully formed values, unlike id where we get a set of words. Plus, no one has asked for it, since tag values are either generated according to some specification (such as Kubernetes-defined labels) or use user input which the user can capitalize any way they want. Also, we have no mechanism for singling out some tags for one format and some tags for another format (e.g. don't touch Kubernetes tags, but make the rest upper case) and I do not want to complexify this further by adding one.

Pascal, camel, snake, and kebab case only make sense for id (where we are given words), but then not even fully there. For id, kebab case is lower with our current default delimiter - (and is in practice our current default since we only feed in lowercase words), and snake is just lower with delimiter _ instead of -

So I say we scale way back. Limit this PR to generatedTagNameCapitalization being one of

  • Title
  • lower
  • UPPER

with default "Title"

Let's keep it simple, get it done, and get it out.

Later, if there is demand, we can add a new PR with idCapitalization being one of

  • lower
  • UPPER
  • Title
  • Pascal
  • camel

and forming the id from the components appropriately. (Remember, kebab and snake are just lower plus choice of delimiter.) We can discuss the finer points of that feature then and there.

@osterman
Copy link
Member

osterman commented Jan 19, 2021

Objectives

  • Introduce a convention that will scale with the various case formats for id and tags outputs

Out of Scope

  • AWS, GCP do not put limitations on the formatting of tag values, so it's out of scope
  • Reformatting tags names to non-trivial cases like pascal and camel
  • additional_tag_map?

Scope

  • Add id_case
    • with a default of lower (current behavior)
    • supported values are one of: lower, upper, title, pascal, camel, inherit* (all in lower-case)
    • respects the delimiter which can be set to an empty string
    • id output respects the id_case (can use map as part of implementation)
  • Add tag_case with a default of title (current behavior)
    • supported values are one of: lower, upper, title, inherit* (all in lower-case; very trivial to support)
    • the tags output respects the tag_case (can use map as part of implementation)
    • the value of the name tag respects the id_case

* inherit means to just pass through the original case (I don't know what this means for context tags). If too confusing, we can omit.

@Nuru
Copy link
Sponsor Contributor

Nuru commented Jan 19, 2021

@osterman I mostly like your revised proposal. As usual, I disagree with your choice of name, in this case inherit, but we can work that out relatively easily, as we always do. My bigger concerns are, for id_case, "respects the delimiter which can be set to an empty string" and "the tags output respects the tag_case" These concern me.

As I pointed out before, kebab andsnake are both just lower plus a specific choice of delimiter. Similary, pascal is the same as title with no delimiter. camel is the only one that is slightly more complex, but again it implies no delimiter.

I would prefer to limit id_case to lower, upper, and title , from which thoughtful users can create snake, kebab, and Pascal case. Yes, this leaves camel out, which is OK with me because it is the most complex.

As for Tags, I think we should not allow any formatting of tag values, and we should not mess with the Tag Names of any tags supplied in the tags variable/context. Rather than "the tags output respects the tag_case", we should have a limited generated_tag_case varaible that takes only lower, upper, or title and it should only apply to the names/keys of the tags the module generates (which is why inherit or none do not really apply). The value of the name tag is, as always, the value of the id output, which is the id formatted according to id_case and truncated by id_length_limit. (This brings up the extra consideration we need to make for the hash we append to the end of a truncated ID. The hash is hexadecimal, so it cannot be considered to work with Pascal or camel case, and we should probably introduce a hypen delimiter in those cases.)

As if we do not have enough to consider, this also brings up another design decision. Currently, id is truncated by cutting off the end and replacing it with the partial hash of the full id. It would conceivably be better if we truncated by cutting out the middle and replacing the middle with a hash of the removed contents. Sort of like the idea behind i18n and k8s. Further, we could convert from hexadecimal to alphabetical, using the letters only, skipping ambiguous b, i, o, s, so we get something like a word that can be shoved into Pascal case.

@Sytten
Copy link

Sytten commented Jan 19, 2021

@osterman Correction, GCP does force tags to start with a lowercase letter.
So for me, it is very important that the none formatting be included or a formatting that only changes the first letter to lowercase.

@osterman
Copy link
Member

@osterman Correction, GCP does force tags to start with a lowercase letter.

@Sytten that's the tag names, but not the tag values. The tag values can be uppercase, etc.

@osterman
Copy link
Member

osterman commented Jan 20, 2021

Okay, I agree with the points raised by @Nuru

Here's my summary for @SweetOps:

  • limit id_case to none, lower, upper, and title , from which thoughtful users can create snake, kebab, and pascal case. This leaves camel out, which is OK for now. Default to lower.

  • Do not allow any formatting of tag values

  • Do not modify tag names of any tags supplied in the tags variable/context.

  • Only modify tag names for generated_tags and generated_tag_name_case variable that takes only lower, upper, or title and it should only apply to the names/keys of the tags the module generates (which is why inherit or none do not really apply). Default to title.

  • The value of the name tag is, as always, the value of the id output, which is the id formatted according to id_case and truncated by id_length_limit.

  • Do not change the way truncation happens today.

@Nuru
Copy link
Sponsor Contributor

Nuru commented Jan 22, 2021

@osterman I left out id_case of none by mistake. I think we should allow it, as users have full control over all the inputs except attributes, so they can implement whatever format they want and we should leave it alone.

Reflecting on it, I think we should name the variable generated_tag_name_case instead of generated_tag_case.

Also, you left out defaults, which for backwards compatibility should be

  • id_case = "lower"
  • generated_tag_name_case = "title"

If it is OK with you, I will edit your comment to incorporate these changes so it remains the reference.

@Nuru
Copy link
Sponsor Contributor

Nuru commented Jan 22, 2021

@SweetOps Please take note of updated requirements here (I made some modifications just now, with @osterman's approval.)

@SweetOps
Copy link
Contributor Author

@osterman @Nuru, take a look pls

@SweetOps
Copy link
Contributor Author

/test all

Copy link
Sponsor Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

Although I still think we should never alter the tag values (or keys) of tags passed in via var.tags, I now think we should apply id_case to the values of the generated tags, so that the values match the corresponding portion of id. What do you think, @osterman ?

examples/autoscalinggroup/context.tf Outdated Show resolved Hide resolved
examples/complete/context.tf Outdated Show resolved Hide resolved
exports/context.tf Outdated Show resolved Hide resolved
exports/context.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@osterman
Copy link
Member

@Nuru @SweetOps: Ok, let's do some "final" =) renaming:

  • id_case to label_value_case (applies to all the values of standard input labels)
  • generated_tag_name_case to label_key_case (applies all the key names of standard input labels)

standard input labels:

  • namespace
  • environment
  • stage
  • name (which is used for id)
  • attributes

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
SweetOps and others added 3 commits January 28, 2021 23:08
@SweetOps
Copy link
Contributor Author

/test all

Copy link
Sponsor Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

Many minor nitpicky changes, because this is a flagship module, but some substantive changes, too.

variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
test/src/examples_complete_test.go Outdated Show resolved Hide resolved
test/src/examples_complete_test.go Show resolved Hide resolved
examples/complete/label8l.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Show resolved Hide resolved
exports/context.tf Outdated Show resolved Hide resolved
exports/context.tf Outdated Show resolved Hide resolved
examples/complete/label8t.tf Outdated Show resolved Hide resolved
examples/complete/label8n.tf Outdated Show resolved Hide resolved
@SweetOps
Copy link
Contributor Author

SweetOps commented Feb 1, 2021

/test all

@SweetOps
Copy link
Contributor Author

SweetOps commented Feb 1, 2021

/test all

@Nuru
Copy link
Sponsor Contributor

Nuru commented Feb 1, 2021

/test all

@Nuru
Copy link
Sponsor Contributor

Nuru commented Feb 1, 2021

/test all

@Nuru Nuru merged commit 6a7c42e into cloudposse:master Feb 1, 2021
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.

7 participants