Skip to content
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 machinery to do client-side RDP license caching #47634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zmb3
Copy link
Collaborator

@zmb3 zmb3 commented Oct 16, 2024

Expose a Cgo interface for writing licenses to the agent's process storage.

@zmb3 zmb3 added backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v16 labels Oct 16, 2024
lib/srv/desktop/rdp/rdpclient/client.go Outdated Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/client.go Outdated Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/client.go Outdated Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/client.go Outdated Show resolved Hide resolved
Expose a Cgo interface for writing licenses to the agent's process
storage.
Comment on lines +826 to +827
case len(license) > 0:
log.InfoContext(context.Background(), "found existing RDP license")
Copy link
Contributor

Choose a reason for hiding this comment

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

what should happen in case of err == nil && len(license) == 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not something I've commonly seen in Go code. It's generally safe to assume that a nil error means we're returning valid data.

Copy link
Contributor

Choose a reason for hiding this comment

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

So why use additional case here? can we skip it and simply log after switch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I follow?

I want to be able to tell from the logs if:

  1. We know there wasn't an existing license.
  2. We know there was an existing license.
  3. We don't know if there was a license or not because we encountered an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now the switch is not exhaustive, case where err == nil and len(license) == 0 will log nothing.
So, we either don't care about the length (we assume it's >0 when err==nil) and we can skip the length check (by either returning early in case of errors above and logging happy path outside of switch or changing last case to default) or we do care and should log something different in that case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough, we can change the last case to a default case instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants