-
Notifications
You must be signed in to change notification settings - Fork 8
Merge snapshot module and create_layout module into one #49
Conversation
cc @lukpueh could you take a look at this? 😄 |
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.
Thanks for working on this, @SolidifiedRay! It would be really great if your commit history made it clearer, what parts of the PR are renames (e.g. test_before_after_filesystem_snapshot.py
--> test_create_layout.py
), or copy-pasting entire functions from one module to another (e.g. the snapshot
function), and what changes/adds functionality (e.g. the create_layout.create_*_rules
functions work differently now and there also seem to be new tests). Do you think you can redraw the git history?
Our commits guideline document has a corresponding paragraph. It also has some good recommendations for commit messages. ;)
Yes, I will do this. Thanks for pointing this out! |
3f7af9b
to
8a1a56c
Compare
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 is looking pretty good, @SolidifiedRay! Just some thoughts on how we can simplify the APIs further. 😄
14283bc
to
8091929
Compare
4f7e1cf
to
1ab873c
Compare
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.
Solid work, @SolidifiedRay! This is looking pretty good. I quite like the tests you have, and the PR needs a few mostly straightforward changes IMO.
Thanks a lot for the review @adityasaky ! I have made most of the changes but I still have some questions and I have replied to your review. |
3d36c15
to
e77c6ee
Compare
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.
LGTM, thanks @SolidifiedRay!
cc @lukpueh any thoughts?
e77c6ee
to
499a883
Compare
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.
Excellent work, @SolidifiedRay and 💯 for extensive testing!! At first I thought I'd only point out some minor style nits, but looking at the tests I did find some interesting issues with the rule creation functions (which is good!! :).
So it seems like we need to be more careful, when an artifact qualifies for multiple rules (see inline comments in tests).
['MATCH', 'bar/bat/four.tgz', 'WITH', 'PRODUCTS', 'FROM', 'first_step'], | ||
['MATCH', 'one.tgz', 'WITH', 'PRODUCTS', 'FROM', 'first_step'], | ||
['MATCH', 'foo/two.tgz', 'WITH', 'PRODUCTS', 'FROM', 'first_step'], | ||
['MATCH', 'three.txt', 'WITH', 'PRODUCTS', 'FROM', 'first_step'], |
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.
Ha! This is rather interesting. Like above, this rule makes the subsequent 'DELETE' rule moot. But unlike above, both rules here are actually meaningful. We should document this issue and add it to the list of artifact rule discussion items in in-toto/specification#4. (cc @SantiagoTorres)
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.
Thanks again for pointing this interesting problem out!
For this problem, I believe the DELETE rule is more important. I think a malicious artifact won't cause many problems if we are going to delete it. But I also believe it is important to let the user know that something wrong happened in the supply chain if there is a mismatch of the artifact.
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.
So you say, if an artifact gets deleted in one step, it doesn't really matter if it matched the product from a previous step? That's an interesting thought, and I do agree with you on a practical level.
However, I still think it is a lot more important to guarantee the integrity of artifacts in transit between steps than within a step. Because I trust the functionary more than the man in the middle. So I would say, match rules across steps should always be prioritized.
Moreover, a DISALLOW *
in the products rules of a step could catch any artifact that was expected to be deleted but wasn't actually deleted.
@SolidifiedRay, would you mind filing an issue on in-toto/docs for this, and see if you can find other such conflicting rules too? Let's discuss this with other in-toto folks there before make a decision here. In the meanwhile we can just warn the caller about these cases and suggest to manually review.
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.
Sure! Here is the link: issue#41.
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.
Excellent write-up of the issue. Thanks, a lot!
499a883
to
68f985c
Compare
Thanks a ton for the review! @lukpueh I have fixed most of the nits that you mentioned. The issue related to the rule creation function is interesting, and I left some ideas under your comments. Could you please check on it? Thanks! |
Thanks for addressing all my comments, and for sharing your suggestions about conflicting rules, @SolidifiedRay! As said above, I think we don't need to find a solution here. Let's just add a warning about the conflict so that we can move forward here. |
68f985c
to
255a1d8
Compare
2ad4a88
to
862e2f8
Compare
862e2f8
to
426b2f6
Compare
I have reimplemented the sort and added a warning about the rule conflict. I really appreciate your code review all the time. Thanks! @lukpueh |
d174897
to
695c82a
Compare
…eate_layout.py Move snapshot function from before_after_filesystem_snapshot.py to create_laytou.py and remove before_after_filesystem_snapshot.py. This snapshot function will be used to enhance the rules generation function in create_layout.py.
2c11fe8
to
d282aa1
Compare
Apart from a tiny nit (see review above) this looks really good. Thanks for your persistence, @SolidifiedRay! @IshaDave, I'd love if you could do some user testing here. Ideally while (re-)creating the layout for the task you are working on (in-toto/in-toto#278). :) |
- Rename snapshot() function to changes_between_snapshots() - Generate DELETE, MODIFY, and CREATE rules generation - For MATCH rules, match only those that already were in the previous step, and allow the rest by name
- Add test_create_material_rules_with_zero_index - Add test_create_material_rules_with_nonzero_index - Add test_create_product_rules
d282aa1
to
43d2792
Compare
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.
🎉
In PR #46, @IshaDave and I created a new module called
before_after_filesystem_snapshot.py
. This module returns a 'snapshot' of a file structure showing which files were unchanged, modified, added, or removed. It also includes a function that generates material rules and product rules based on this snapshot.In this PR, I move the functions from
before_after_filesystem_snapshot.py
tocreate_layout.py
. Thecreate_layout.py
will now utilize the snapshot function to generate more complex material rules and product rules, includingMATCH
,ALLOW
,MODIFY
,DELETE
,CREATE
,DISALLOW
.You can also read issue #48 for more ideas.