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 context #25

Merged
merged 23 commits into from
Jul 24, 2018
Merged

Add context #25

merged 23 commits into from
Jul 24, 2018

Conversation

osterman
Copy link
Member

@osterman osterman commented Jun 28, 2018

what

  • Support passing a label's context between label modules

why

  • DRY

demo

module "label1" {
  source     = "../../"
  namespace  = "Namespace"
  stage      = "Stage"
  name       = "Name1"
  attributes = ["1", "2", "3", ""]
  tags       = {
    "SomeKey" = "SomeValue"
  }
}

module "label2" {
  source     = "../../"
  context    = "${module.label1.context}"
  name       = "Name2"
}

todo

  • figure out how to inherit extra tags from var.tags <--- need help

@osterman osterman added the wip Work in Progress: Not ready for final review or merge label Jun 28, 2018
@osterman
Copy link
Member Author

osterman commented Jun 28, 2018

This is working and produces the following output. I just don't know how to inherit the additional tags from the parent's context.

Outputs:

label1_attributes = 1-2-3
label1_context = {
  attributes = 1-2-3
  delimiter = -
  name = name1
  namespace = namespace
  stage = stage
  tags = map[SomeKey:SomeValue Name:namespace-stage-name1-1-2-3 Namespace:namespace Stage:stage]
}
label1_id = namespace-stage-name1-1-2-3
label1_name = name1
label1_namespace = namespace
label1_stage = stage
label1_tags = {
  Name = namespace-stage-name1-1-2-3
  Namespace = namespace
  SomeKey = SomeValue
  Stage = stage
}
label2_attributes = 1-2-3
label2_context = {
  attributes = 1-2-3
  delimiter = -
  name = name2
  namespace = namespace
  stage = stage
  tags = map[Namespace:namespace Stage:stage Name:namespace-stage-name2-1-2-3]
}
label2_id = namespace-stage-name2-1-2-3
label2_name = name2
label2_namespace = namespace
label2_stage = stage
label2_tags = {
  Name = namespace-stage-name2-1-2-3
  Namespace = namespace
  Stage = stage
}

main.tf Outdated
"Stage" = "${local.enabled ? local.stage : ""}"
}

tags = "${merge(local.generated_tags, var.tags)}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to incorporate the var.context["tags"] here some how

Choose a reason for hiding this comment

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

it doesn't seem possible :(

@dennybaa
Copy link

@osterman Hey, it's an awesome idea to have context.

Unfortunately I also don't see how it's possible with HCL1. We should always try to remember that scalars such as maps/list have always to be flat. Basically scalars must:

  • be homogeneous
  • include only simple types as string/integer/boolean

These limitations will be hopefully resolved in 0.12 if it includes HCL2...

In other words now all the variables and all the outputs should be flat and homogeneous. With the introduced context you break this rule.

Regarding non-flat scalars, the only workaround possible now is leveraging locals for intermediate computation, like bellow:

locals {
  non_flat_map = {
    map1 = "${var.map1}"  # var.map1 should be flat
    map2 = "${var.map2}"  # var.map2 should be flat
  }

  target_map = "${local.non_flat_map[var.choice ? "map1": "map2"]}"
}

output "map" {
  value = "${local.target_map}"
}

@Jamie-BitFlight
Copy link
Contributor

Jamie-BitFlight commented Jun 28, 2018

This code:

module "label1" {
  source     = "../../"
  namespace  = "CloudPosse"
  stage      = "Production"
  name       = "Winston"
  attributes = ["fire", "water", "earth", "air"]

  tags = {
    "City"        = "Dublin"
    "Environment" = "Private"
  }
}

module "label2" {
  source  = "../../"
  context = "${module.label1.context}"
  name    = "Charlie"

  tags = {
    "City"        = "London"
    "Environment" = "Public"
  }
}
output "label1" {
  value = {
    id         = "${module.label1.id}"
    name       = "${module.label1.name}"
    namespace  = "${module.label1.namespace}"
    stage      = "${module.label1.stage}"
    attributes = "${module.label1.attributes}"
  }
}

output "label2" {
  value = {
    id         = "${module.label2.id}"
    name       = "${module.label2.name}"
    namespace  = "${module.label2.namespace}"
    stage      = "${module.label2.stage}"
    attributes = "${module.label2.attributes}"
  }
}

output "label1_tags" {
  value = "${module.label1.tags}"
}

output "label1_context" {
  value = "${module.label1.context}"
}

output "label2_tags" {
  value = "${module.label2.tags}"
}

output "label2_context" {
  value = "${module.label2.context}"
}

Makes this output now

Outputs:

label1 = {
  attributes = [fire water earth air]
  id = cloudposse-production-winston-fire-water-earth-air
  name = winston
  namespace = cloudposse
  stage = production
}
label1_context = {
  attributes = [fire water earth air]
  delimiter = [-]
  name = [winston]
  namespace = [cloudposse]
  stage = [production]
  tags_keys = [City Environment Name Namespace Stage]
  tags_values = [Dublin Private cloudposse-production-winston-fire-water-earth-air cloudposse production]
}
label1_tags = {
  City = Dublin
  Environment = Private
  Name = cloudposse-production-winston-fire-water-earth-air
  Namespace = cloudposse
  Stage = production
}
label2 = {
  attributes = [fire water earth air]
  id = cloudposse-production-charlie-fire-water-earth-air
  name = charlie
  namespace = cloudposse
  stage = production
}
label2_context = {
  attributes = [fire water earth air]
  delimiter = [-]
  name = [charlie]
  namespace = [cloudposse]
  stage = [production]
  tags_keys = [City Environment Name Namespace Stage]
  tags_values = [London Public cloudposse-production-charlie-fire-water-earth-air cloudposse production]
}
label2_tags = {
  City = London
  Environment = Public
  Name = cloudposse-production-charlie-fire-water-earth-air
  Namespace = cloudposse
  Stage = production
}

@Jamie-BitFlight
Copy link
Contributor

@dennybaa I got it working

Copy link
Contributor

@Jamie-BitFlight Jamie-BitFlight left a comment

Choose a reason for hiding this comment

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

LGTM

main.tf Outdated
# Merge the map of empty values, with the variable context, so that context_local always contains all map keys
context_local = "${merge(local.context_context, var.context)}"
# Only maps that contain all the same attribute types can be merged, so they have been set to list
context_context = {
Copy link
Member Author

Choose a reason for hiding this comment

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

can we rename this? maybe call it null_context?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm renaming it to context_struct, since it is the structure of context with all variables.

main.tf Outdated
selected_stage = ["${compact(concat(local.context_local["stage"], list(var.stage)))}"]
stage = "${lower(join("", split(" ", local.selected_stage[0])))}"
selected_attributes = ["${distinct(compact(concat(var.attributes, local.context_local["attributes"])))}"]
attributes = "${split("~^~", lower(join("~^~", local.selected_attributes)))}"
Copy link
Member Author

Choose a reason for hiding this comment

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

let's call ~^~ something (E.g. unique_separator) - either a variable or a local.

sarkis
sarkis previously approved these changes Jun 28, 2018
Jamie Nelson added 2 commits June 29, 2018 00:10
…bel into add-context

* 'add-context' of github.com:cloudposse/terraform-null-label:
  Migrate to README.yaml format (#24)
@osterman osterman removed the wip Work in Progress: Not ready for final review or merge label Jun 28, 2018
@osterman
Copy link
Member Author

@dennybaa - please have another look at this. It's not pretty, but it sets out to accomplish what we wanted to do. I'm pretty sure we'll end up merging this.

@dennybaa
Copy link

@osterman Wow. That's a lot of effort, this might be a good example of that how to mitigate passing non-flat structures :)

It would be nice if you also switch to null_data_source this reduces output rubbish during plan/apply, and specifically made for this, in other words resource is an overkill here.

This would work now, though tags passing seemed reasonably easier and if I'm not mistaken achieved the same goal. My personal opinion that terraform is not yet ready for things we are trying to do... Don't get me wrong but it doesn't feel like an easy way. Though I really admire your approach and implementation 👍

@dennybaa
Copy link

So I still think, taking an approach of calling tags Name/Namespace/Stage as generated_tags. IMO it still looks neater for me to have an option like exclude_generated_tags, because it will generally keep locals computations simpler.

@Jamie-BitFlight
Copy link
Contributor

I think that this will get far easier in tf 0.12

goruha
goruha previously approved these changes Jul 16, 2018
Copy link
Member

@goruha goruha left a comment

Choose a reason for hiding this comment

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

Looks complicated, but not too much.
Eventually we will simplify that for 0.12 tf

@dennybaa
Copy link

@osterman yeah, it looks a bit complicated, but what we can do :) The module is very usable now 👍 for it.

@dennybaa
Copy link

@osterman osterman added the enhancement New feature or request label Jul 24, 2018
…ment as an optional variable. Swapped out null_resource for null_data_source, which reduces some of the output noise but keeps the funtionality the same.
@Jamie-BitFlight Jamie-BitFlight merged commit 6edc9b2 into master Jul 24, 2018
@Jamie-BitFlight Jamie-BitFlight deleted the add-context branch July 24, 2018 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants