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

in_toto_verifylib: fix require/disallow queue #280

Merged
merged 3 commits into from
May 13, 2019

Conversation

SantiagoTorres
Copy link
Member

Fixes issue #: None

Description of the changes being introduced by the pull request:

This reverts commit 21129a6 and
replaces the consumed = artifacts_queue with consumed = set(), the
reason as to why is that a require rule passing overwrites all the queue
with a set() call...

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@coveralls
Copy link

coveralls commented Apr 20, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 5056a13 on fix-require-queue-modification into cc73474 on develop.

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 the fix, @SantiagoTorres! I referenced the PR in the list of artifact rule related PRs (in-toto/specification#4).

What do you think about defining the empty set only once above the if/else cascade? Similar to how we do it in our in-toto-golang (where the compiler forced us to do this right in the first place).

@SantiagoTorres
Copy link
Member Author

I was hesitant to do that just to avoid it to allocate a new set() for every iteration. I wonder if what we'd like instead is to declare it as an empty set outside the for?

@lukpueh
Copy link
Member

lukpueh commented Apr 24, 2019

Okay, fair enough. Not sure I understand your alternative proposition. We can't declare consumed outside the loop as it gets overridden by match/create/delete/modify/allow rules. Or did you mean declare an empty_set = set() and in disallow/require do something like consumed = empty_set?

I think my preferred solution would be one of,

  • as it is now,
  • what I suggested above (despite the unnecessary allocations),
  • in match/create/delete/modify/allow replace
    consumed = verify_*_rule(... with artifacts_queue -= verify_*_rule(...,
  • modify verify_disallow_rule and verify_require_rule to return an empty set too and also assign it to consumed

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.

Let's merge this soon. Let me know what you think of my suggestions (above). If none if it seems preferable we can merge as is.

@SantiagoTorres
Copy link
Member Author

Let's do what you suggested above despite the allocations.

SantiagoTorres and others added 3 commits May 13, 2019 16:51
This reverts commit 21129a6 and
replaces the consumed = artifacts_queue with consumed = set(), the
reason as to why is that a require rule passing overwrites all the queue
with a set() call...

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
"Disallow" and "require" rules don't consume artifacts in the
queue, but still need to initialize a consume set.

This commit summarizes the initialization in the "disallow" and
"require" elif blocks (readability over performance).

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Update docstring to mention newly added require rule.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh force-pushed the fix-require-queue-modification branch from 67c5b2d to 5056a13 Compare May 13, 2019 14:51
@lukpueh
Copy link
Member

lukpueh commented May 13, 2019

@SantiagoTorres, I took over your branch. And made the small modification we talked about. I hope that's okay? I also fixed the outdated docstring. Can you take a quick look at it and greenlight?

@SantiagoTorres
Copy link
Member Author

LGTM. Thanks!

@SantiagoTorres SantiagoTorres merged commit f4cea28 into develop May 13, 2019
@SantiagoTorres SantiagoTorres deleted the fix-require-queue-modification branch October 31, 2019 16:17
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