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

Feature/support tagging #181

Merged
merged 3 commits into from Mar 16, 2019

Conversation

Projects
None yet
3 participants
@theburningmonk
Copy link
Collaborator

theburningmonk commented Mar 14, 2019

PR for #166 now that CloudFormation supports tagging for Step Functions

theburningmonk added some commits Mar 14, 2019

@theburningmonk theburningmonk requested a review from horike37 Mar 14, 2019

@noahw3

This comment has been minimized.

Copy link

noahw3 commented Mar 14, 2019

Looks like this matches the cloudformation template exactly, but it might be nicer syntactically to define the tags as a dictionary (like how lambdas work) and then internally convert it to the array of key/value pairs?

Ie instead of

tags:
  -
    Key: keyname1
    Value: value1
  -
    Key: keyname2
    Value: value2

more like

tags:
  keyname1: value1
  keyname2: value2
@theburningmonk

This comment has been minimized.

Copy link
Collaborator Author

theburningmonk commented Mar 14, 2019

@noahw3 I thought about that too, but I have actually seen teams use : as delimiter in the key. There's nothing stopping us from still using : as delimiter though, e.g. my:namespace:for:tags:value => { Key: "my:namespace:for:tags", Value: "value" } but it might look a bit old.

But what if Value is an ARN which is also delimited by :? It would break our parsing. Open to suggestions as to how we might address that.

@noahw3

This comment has been minimized.

Copy link

noahw3 commented Mar 14, 2019

If a special character is needed in the actual value (for either key or value), quoting it should be sufficient? YAML supports that.

tags:
  keyname1: "value1:append:all:the:things:with:colons:"
  "key:name2": value2
@theburningmonk

This comment has been minimized.

Copy link
Collaborator Author

theburningmonk commented Mar 14, 2019

@noahw3 ahh, I completely misunderstood you! You meant treat it as an object instead of array, of course, that makes sense :-D

@noahw3

noahw3 approved these changes Mar 15, 2019

@horike37
Copy link
Owner

horike37 left a comment

LGTM @theburningmonk 👍 💯 and thank you for the suggestion @noahw3 😄

@horike37 horike37 merged commit b59baed into horike37:master Mar 16, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@theburningmonk theburningmonk deleted the theburningmonk:feature/support_tagging branch Mar 16, 2019

@theburningmonk

This comment has been minimized.

Copy link
Collaborator Author

theburningmonk commented Mar 16, 2019

🎉 This PR is included in version 1.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.