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.
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
[Flyte][3][flytepropeller][Attribute Access][flytectl] Binary IDL With MessagePack #5763
[Flyte][3][flytepropeller][Attribute Access][flytectl] Binary IDL With MessagePack #5763
Changes from 2 commits
c0bc45f
c9616ca
6e6d449
267852b
626f9c3
f68f75c
52a7595
548fdba
b306b4b
c8dd0a0
34914db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 110 in flytepropeller/pkg/controller/nodes/attr_path_resolver.go
Codecov / codecov/patch
flytepropeller/pkg/controller/nodes/attr_path_resolver.go#L110
Check warning on line 116 in flytepropeller/pkg/controller/nodes/attr_path_resolver.go
Codecov / codecov/patch
flytepropeller/pkg/controller/nodes/attr_path_resolver.go#L113-L116
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.
What will GetStringValue() return if if the attr is
foo[0]
? shouldn't it fail in that case and say "you can't index into a struct" or something ?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, just fix this case here.
https://github.com/flyteorg/flyte/pull/5763/files#diff-ee7f936e440a7e043b3bc7acb4ea255ba991dea8f3144d24ab276c3a292de018R136-R164
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.
ditto here. If it's
foo.bar
GetIntValue() will return 0 I presume? it should fail here tooThere 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,
GetIntValue() will return 0
, butfoo.bar
will go to the upmap
case, so it will calledattr.GetStringValue
to get thebar
attributeCheck warning on line 142 in flytepropeller/pkg/controller/nodes/attr_path_resolver.go
Codecov / codecov/patch
flytepropeller/pkg/controller/nodes/attr_path_resolver.go#L142
Check warning on line 150 in flytepropeller/pkg/controller/nodes/attr_path_resolver.go
Codecov / codecov/patch
flytepropeller/pkg/controller/nodes/attr_path_resolver.go#L148-L150
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.
This case can never happen, right? since you checked for that early on
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, I can remove it