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

Enable StatusString for all readers #50

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Conversation

greedy52
Copy link

@greedy52 greedy52 commented Nov 20, 2023

Related

In #37, a readerOpt was added as an option to enable StatusString to workaround UT issues. However, this means the default connection pool does NOT have this flag ENABLED, thus making the original StatusString fix USELESS. 😭

This PR reverted the readerOpt part and fixed the other UTs properly.

I did consider pushing the StatusString fix upstream but it's clear that this breaks backward compatibility as cmd.Val() will return a proto.StatusString instead of string.

Will need to revisit later to see if possible to make an upstream acceptable version of the fix or maybe fix this entirely on the Teleport side without driver change.

Also there are some testing escapes. Will add a "COMMAND DOCS" test at a higher level in teleport repo. And for e2e testing, we should always test out a user with full permission (I didn't catch this because my users do not have COMMAND DOCS permission to trigger this case)

@greedy52 greedy52 self-assigned this Nov 20, 2023
@greedy52
Copy link
Author

friendly ping @jentfoo 😃

Copy link

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

Good catch @greedy52, thank you for continuing to think on this issue.

@greedy52 greedy52 merged commit bd8aa68 into master Nov 27, 2023
6 checks passed
@greedy52 greedy52 deleted the STeve/fix_reader_opt branch November 27, 2023 21:11
greedy52 added a commit that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants