Skip to content

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Oct 25, 2021

I noticed that we should return an error if the logs API is not supported instead of continuing the subscribe function.

@estolfo estolfo requested a review from astorm October 25, 2021 11:41
@github-actions github-actions bot added the aws-λ-extension AWS Lambda Extension label Oct 25, 2021
@ghost
Copy link

ghost commented Oct 25, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-25T11:41:05.013+0000

  • Duration: 7 min 37 sec

  • Commit: 328927c

Test stats 🧪

Test Results
Failed 0
Passed 48
Skipped 0
Total 48

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

👍 Looks like a reasonable precaution/fix. Approving.


if resp.StatusCode == http.StatusAccepted {
log.Println("Logs API is not supported. Is this extension running in a local sandbox?")
return nil, errors.Errorf("Logs API is not supported in this environment")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I'd missed this in the previous PRs, but it surprised me that a 202 response code means "we see you but this request won't do anything", but it's documented right here: https://docs.aws.amazon.com/lambda/latest/dg/runtimes-logs-api.html

Considering this an error case makes sense and is appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code was from the aws examples so I missed checking it closely...

@estolfo estolfo merged commit 7bfe00a into main Oct 26, 2021
@estolfo estolfo deleted the estolfo/logsapi-unsupported branch November 15, 2021 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws-λ-extension AWS Lambda Extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants