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

encode return value as integer instead of string #36

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

shibumi
Copy link
Contributor

@shibumi shibumi commented Jul 24, 2020

The in-toto python reference implementation encodes the return-value
field of our link files as integer. Our specification specified this
field as string value. This commit fixes this inconsistency and
assigns the right data type to the return-value field.

CC: @SantiagoTorres @lukpueh @trishankatdatadog

@shibumi
Copy link
Contributor Author

shibumi commented Jul 24, 2020

One open question is, how do we want to handle null values? Right now I have just assigned null to the example of the link file, underlining that return-value is an integer object and not a string (the value " " would be wrong in this case). Practically, I think we never have no return value, right?

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

LGTM. I agree with @shibumi that it makes sense to recommend int and null, especially since that's what we expect in the reference implementation.

in-toto-spec.md Outdated Show resolved Hide resolved
in-toto-spec.md Outdated Show resolved Hide resolved
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Maybe @SantiagoTorres wants to make the final call. :)

The in-toto python reference implementation encodes the return-value
field of our link files as integer. Our specification specified this
field as string value. This commit fixes this inconsistency and
assigns the right data type to the return-value field.

Furthermore we remove a "return-value" line, outside of
of an actual byproducts object. May has been a remainder
of the signed part.
@shibumi
Copy link
Contributor Author

shibumi commented Jul 28, 2020

@SantiagoTorres @lukpueh I have force pushed a new version + I have modified the commit message, that it's clear we have also removed one return-value line that was a remainder.

@lukpueh
Copy link
Member

lukpueh commented Oct 27, 2020

Maybe @SantiagoTorres wants to make the final call. :)

re-ping @SantiagoTorres

Copy link
Member

@adityasaky adityasaky left a comment

Choose a reason for hiding this comment

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

I think this change looks good to me, and would represent the present behaviour in in-toto-python and in-toto-golang. Would we need to make any changes to record return-value as null if it isn't recorded?

@shibumi
Copy link
Contributor Author

shibumi commented Jan 22, 2021

@lukpueh still need to withdraw his request for changes or click on 'approve', if this should get merged :)

@lukpueh
Copy link
Member

lukpueh commented Jan 25, 2021

Would we need to make any changes to record return-value as null if it isn't recorded?

Good question. in-toto-run always records a return-value in both implementations. in-toto-record (only available in the reference implementation) returns an empty byproducts field. Given that this is only a recommendation, I think we are fine either way.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this, @shibumi!

Copy link
Member

@adityasaky adityasaky left a comment

Choose a reason for hiding this comment

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

Agreed, @lukpueh. LGTM!

@lukpueh lukpueh merged commit ebd439d into in-toto:master Jan 27, 2021
@shibumi shibumi deleted the shibumi/fix-return-value branch January 27, 2021 15:19
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