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

Fix unmarshalling of CreateDatabaseUserMode #49995

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

bl-nero
Copy link
Contributor

@bl-nero bl-nero commented Dec 10, 2024

This is required to support this field in the new role editor, since data returned from /webapi/yaml/stringify contains numeric enum values. When fed into PUT /webapi/roles:name, it then resulted in an error.

@bl-nero bl-nero added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Dec 10, 2024
@@ -2253,8 +2253,23 @@ func (h CreateDatabaseUserMode) encode() (string, error) {
func (h *CreateDatabaseUserMode) decode(val any) error {
var str string
switch val := val.(type) {
case int32:
return trace.Wrap(h.setFromEnum(val))
case int64:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to handle so many distinct types? Do we really expect inputs to vary this much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codingllama Frankly, I don't know; I think that only floats would do, but I noticed that the other type (CreateHostUserMode) has it defined this way, so I decided to follow it to maintain consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you decode from json it'll always be a float64, so I found the other types curious. Technically it doesn't hurt so I won't block you for this.

api/types/role_test.go Outdated Show resolved Hide resolved
api/types/role_test.go Outdated Show resolved Hide resolved
api/types/role_test.go Outdated Show resolved Hide resolved
@bl-nero
Copy link
Contributor Author

bl-nero commented Dec 11, 2024

@codingllama I also changed the other test cases around for consistency.

@bl-nero bl-nero requested review from codingllama and removed request for fspmarshall December 11, 2024 17:25
@bl-nero bl-nero enabled auto-merge December 11, 2024 17:43
@bl-nero bl-nero added this pull request to the merge queue Dec 11, 2024
Merged via the queue into master with commit e73e8aa Dec 11, 2024
41 checks passed
@bl-nero bl-nero deleted the bl-nero/role-unmarshalling-fixes branch December 11, 2024 18:14
@public-teleport-github-review-bot

@bl-nero See the table below for backport results.

Branch Result
branch/v17 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants