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
Add support for Kinesis logging endpoint #177
Add support for Kinesis logging endpoint #177
Changes from all commits
75eccff
944cf3d
cacda3a
8bbe7a1
42e6b99
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 sure 'region' is required? It doesn't look to be in the API docs example (nor in Go-Fastly).
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, it is required. The configuration validation will fail if it is not present. I see what you mean in the case of
go-fastly
. I followed the existing patterns of other logging endpoints when I did that work and they all seem to only haveServiceId
andServiceVersion
as required. My guess is that the enforcement of required fields was deferred to the config validation, but maybe we want to revisit that.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.
Question:
I presume 'config validation' refers to the upstream API's own validation, would that be correct?
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 see Go-Fastly currently doesn't define validation for the 'region' field, but if we're saying the API itself is expecting a region field otherwise it'll return an error, then we should ensure a validation check for the 'region' field is added into Go-Fastly (I have an existing PR open I can add this to).
But for the time being, this is fine to be marked as
.Required()
considering Go-Fastly doesn't currently have logic to catch the missing field.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.
Also, just FYI (most of which will be obvious to you): my perspective on this is that we have three tools at play here:
We should be able to trust the API to always have the right validation in place, but (as you know) by having validation outside of the API it means we can prevent slowing down a client trying to carry out operations that otherwise we know are just going to fail by the time they reach the API layer, so short-circuiting those requests is better for the client as far as the 'feedback loop' is concerned but also good for the API because it doesn't have to handle the request in the first place.
Considering Go-Fastly and CLI sit at effectively the same level (i.e. they exist on the client) we need to decide which 'tool' is responsible for handling validation of obvious issues with a client request, and for me that should be Go-Fastly as it's the thing that's ultimately fronting the API.
So my hope is that with the work @doramatadora has been doing on the new OpenAPI specification it will mean we can move to a place where we more concretely know what is a required field and have Go-Fastly handle validation and thus reduce duplicating logic in the CLI (although admittedly at this point in time the OpenAPI spec for Kinesis logging still doesn't indicate that 'region' is a required field nor does it show it as part of its example code so that would likely need updating).
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.
That's correct.
If you send me the service id in slack I can enable this for you.
I'll also follow-up on the docs side of things and open a change request if needed to have it indicate
region
as required.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 think we should create a new
/pkg/text/lines.go
to make it easier to render output for newline information (like logging endpoints) and then we'd also be consistent with other packages in the CLI that use the text package.Something generic like this would suffice: https://play.golang.org/p/csgKDzLZPeu
Which means you could do something like:
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.
@phamann what do you think about this...
If we combined this generic implementation (see my comment above) with
textio.NewPrefixWriter
then we could probably end up reducing quite a few of the existingPrint<T>
functions (e.g.PrintBackend
,PrintDictionaryItemKV
etc).You could even take it a step further and add in the ability to pass in an optional callback function for those edge cases such as
PrintDictionaryItem
where they only print lines that have values (although to be fair that's probably logic you'd shift back up into the consuming package). But I'm just 'brain dumping' ideas at the moment.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.
👍 to this idea from me. I'd say this is probably out of scope for this PR, but I think it would be a good change to implement broadly.
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.
Agree on both accounts:
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.
👍🏻 👌🏻