Skip to content

Conversation

@Jamie-BitFlight
Copy link
Contributor

what

Updated the label module to 0.11.1, allowed the use of context for a DRY'er finish

why

Allows for fewer repeated variables. Allows users of the label module to pass context into this module.

@osterman osterman requested a review from aknysh June 6, 2019 17:57
Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

I'm okay with splitting out the label into a separate file, but maybe combine them into one file?

Co-Authored-By: Andriy Knysh <[email protected]>
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

see comments

@Jamie-BitFlight
Copy link
Contributor Author

Updated to comply

osterman
osterman previously approved these changes Jun 7, 2019
@osterman osterman requested a review from aknysh June 7, 2019 04:18
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @Jamie-BitFlight
please see comments

…. Added all variables that the Label module uses.
label.tf Outdated
variable "label_order" {
type = "list"
default = []
description = "[Label Module] The naming order of the id output and Name tag"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = "[Label Module] The naming order of the id output and Name tag"
description = "The naming order of the id output and Name tag"

I think we don't need to use [Label Module] in the descriptions, it makes it more difficult to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. I was wanting a way to categorise the label modules inputs, so that they could be separated from the dynamic subnets module variables.
Is there another way we could categorise it in the readme @aknysh?

Copy link
Member

Choose a reason for hiding this comment

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

@Jamie-BitFlight
The README generator reads all descriptions and puts them into README (so no fast way to categorize them).
I think we don't need to categorize with [Label Module], the users of the module (and other CloudPosse modules) are already familiar with the label attributes, so there is no reason to separate them from other module's variables

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @Jamie-BitFlight
please see comments

@Jamie-BitFlight
Copy link
Contributor Author

Jamie-BitFlight commented Jun 7, 2019 via email

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@aknysh aknysh merged commit f5cefb4 into cloudposse:master Jun 7, 2019
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.

3 participants