Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Unified service tagging for AWS Lambda #9374
Unified service tagging for AWS Lambda #9374
Changes from 5 commits
f21e647
43cc38a
8e17e0d
f80ede5
e48e19d
c26ca83
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we adding functionality to support adding tags on all functions in your
template.yml
? Without additional context, this looks like a typo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this supported? https://github.com/DataDog/datadog-cloudformation-macro/tree/master/serverless#aws-sam Did I miss anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant specifically for the version tag since that's not listed in the example here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we need to update the macro to support the version tag first, and then come back to update this doc, since the version tag is not yet supported by the macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a section heading here? If it's just one section, we can leave this as a note or bold it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only a subsection under the Configuration section, but I can go either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note - can we remove this from the main installation steps into a separate file? I think since this doesn't apply to most customers this will come off as confusing ("This seems complicated, I don't know if I should set this up") and makes our happy path installation instructions longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove this for all runtimes, a potential solution is to add a new Code Signing page under the "Serverless Integrations" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can shorten the paragraph and link it to a separate page, but I don't think we should remove this paragraph completely here, because for customers having code signing enforcement enabled this is a must-do step. Of course, it's a relatively new feature so it's less important right now, but I don't know if it's going to be adopted more widely in the future. Either way, I think this should be a separate PR, and probably we should also consult @nhinsch's opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding from takes from serverless devs online is this something more niche that people in enterprises know to look for to check an audit/compliance checkbox (as opposed to something they aren't sure whether or not they need), so I think there's more benefit to putting this in a separate page so our instructions are more straightforward.