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

FDO 1.2 Delegate Support #24

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

Conversation

bkgoodman
Copy link

No description provided.

Signed-off-by: Brad Goodman <brad@bradgoodman.com>
@bkgoodman
Copy link
Author

Wondering if this should go into a seperate/new "1.2" branch...?

@ben-krieger
Copy link
Member

Wondering if this should go into a seperate/new "1.2" branch...?

Yes, I'll create a 1.2 branch to target. I think it would also be good to work on a branch that we can both push to, because this is such a large contribution that I'd rather just directly help with code cleanup and refactoring than ask you to make a million tiny changes via code review comments.

@@ -101,3 +101,42 @@ func TestSignAndVerify(t *testing.T) {
}
})
}

func TestSomethingThatFailedSignatureVerificationOnceInCIForUnknownReasons(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This got re-added and can be removed.

@@ -752,9 +752,11 @@ func (d *Decoder) decodeArray(rv reflect.Value, additional []byte) error {
return d.decodeArrayToStruct(rv, additional)
case reflect.Slice, reflect.Array:
return d.decodeArrayToSlice(rv, additional)
case reflect.Map:
return d.decodeMap(rv, additional)
Copy link
Member

Choose a reason for hiding this comment

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

This is new functionality and needs explicit tests.

Is decoding an array into a map something we really want, though? It seems to violate the principle of least surprise. If you try to decode an array and provide a map pointer, is that more likely intended or a mistake?

It's also just not that hard to convert a slice of pairs into a map with Go code after decoding... If nothing else, we may want this to be a configurable option of the decoder that is off by default.

Where is this functionality required in this PR?

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.

2 participants