-
Notifications
You must be signed in to change notification settings - Fork 131
Add period to the logging https endpoint configuration. #749
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
Conversation
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.
Thanks for your contribution, @bowrocker! 🙌
I noticed a couple of issues we’ll need to address before moving this forward:
- I don’t see
period
documented in the official Fastly HTTPS logging API reference: https://www.fastly.com/documentation/reference/api/logging/https/. Since this is an open source repository, we can only support fields that are officially documented. - The
period
field also doesn’t appear to be part of the API response body.
Additionally, fixtures should not be updated manually. Instead:
- Remove all fixtures for the resource you’ve modified.
- Set your API token with
FASTLY_API_KEY="your_token"
. - Run
make test
to regenerate the fixtures.
Before we can merge your changes, the new field needs to be officially documented and included in the API response. Once that’s confirmed, we’ll be in a good position to proceed.
Let me know if you have any questions or if you’d like help with the fixture regeneration process. Thanks again!
@bowrocker please notify me as soon as the docs changes are live. At this point we're just waiting on the docs changes. Thank you! |
@philippschulte the api-documentation API for this has been merged: https://github.com/fastly/api-documentation/pull/1192 We are looking to merge this as soon as possible, so please let me know what else you need. |
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.
LGTM @philippschulte any thoughts before merging?
Change summary
All Submissions:
New Feature Submissions:
Changes to Core Features:
User Impact
This change maintains the status quo in the case where customers make no changes to their HTTPS endpoint configuration and only leverages the new functionality if the customer explicitly opts into it. The risk of impact is low because customers using the HTTPS logging endpoint will be unaffected by this change by default.
Risk: Low