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.
Add a ClientVPN connection handler request/response definition #343
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 a ClientVPN connection handler request/response definition #343
Changes from all commits
59458b3
d0ff073
4c8bd6e
89dc954
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.
small nitpick: do you need to set either of these? The nil value for a string is "" and it might be ok for
PostureComplianceStatuses
to be nil? I only ask because omitting them might be cleaner.[edit] https://docs.aws.amazon.com/vpn/latest/clientvpn-admin/connection-authorization.html suggests these two fields are "required" so I definitely wouldn't
omitempty
them but it might still be ok for them to be""
andnil
(their zero values).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've written a test like this to see how I can make this happen:
for the string value that is easy; I can just drop it and it will be
""
, but the array is not so easy to get marshalled as{}
; if I understand golang/go#37711 correctly the choice is either afunc NewClientVPNConnectionHandlerResponse
or a custom marshaller that handles this; both feel like they depart from how the rest of the library is used a bit, although there seems to be functions for DynamoDB attributes as well.Anything you'd suggest? I somehow feel it's easier to keep this in the sample and hopefully people will use the sample to then end up with the right data structure? The alternative that feels clean as well is the function that creates an "empty" object as it could also create the SchemaVersion as well and we can drop the version from the sample code; that would gain something at least.
What do you think?
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.
should marshal as:
The question is: is this ok? Or does it cause failures. Does it need to be:
If it does then yea, we'll need to keep the:
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 tested with
and
and they both fail with an "internal" error. I think the
{}
is indeed required to be returned to ClientVPN.Would you think a
func NewClientVPNConnectionHandlerResponse
would be good or should we leave the example as-is?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.
Ah, that stinks. It's
[]
not{}
right?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.
Yes, sorry; it's an empty array. Even in JSON. The behavior with ClientVPN I've tested is that both code snippets above in #343 (comment) don't result in a successful authentication; whereas if I use the original snippet that initializes an empty array instead of nil it works.
So I think we either need to include this in the sample or provide a function that correctly initializes the response struct.