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

Allow notebooks to be trusted without triggering a save #2718

Merged
merged 1 commit into from Aug 7, 2017

Conversation

Projects
None yet
4 participants
@Madhu94
Contributor

Madhu94 commented Aug 1, 2017

Closes #195.

Trust should now add the notebook's signature without needing to trigger a save. Save will still trust the notebook, after checking and if it has not already been trusted.

I have not added an automatic refresh after trust, let me know if you think this would be a good idea.

@Madhu94 Madhu94 closed this Aug 2, 2017

@Madhu94 Madhu94 reopened this Aug 2, 2017

@minrk

Thanks! I left a few comments inline.

Show outdated Hide outdated notebook/services/contents/filemanager.py
Show outdated Hide outdated notebook/services/contents/handlers.py
Show outdated Hide outdated notebook/services/contents/manager.py
Show outdated Hide outdated notebook/static/notebook/js/notebook.js
@Madhu94

This comment has been minimized.

Show comment
Hide comment
@Madhu94

Madhu94 Aug 2, 2017

Contributor

@minrk Thanks for the feedback! Took care of those comments.

Contributor

Madhu94 commented Aug 2, 2017

@minrk Thanks for the feedback! Took care of those comments.

@minrk

Thanks for the quick turnaround!

@@ -443,7 +443,9 @@ def check_and_sign(self, nb, path=''):
if self.notary.check_cells(nb):
self.notary.sign(nb)
else:
self.log.warning("Saving untrusted notebook %s", path)

This comment has been minimized.

@minrk

minrk Aug 2, 2017

Member

Maybe leave this at "Notebook %s is not trusted."

@minrk

minrk Aug 2, 2017

Member

Maybe leave this at "Notebook %s is not trusted."

Show outdated Hide outdated notebook/static/notebook/js/notebook.js

@Madhu94 Madhu94 closed this Aug 2, 2017

@Madhu94 Madhu94 reopened this Aug 2, 2017

@Madhu94

This comment has been minimized.

Show comment
Hide comment
@Madhu94

Madhu94 Aug 2, 2017

Contributor

@minrk In addition to the changes you requested, I added this, I think the check makes sense here.

Contributor

Madhu94 commented Aug 2, 2017

@minrk In addition to the changes you requested, I added this, I think the check makes sense here.

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Aug 4, 2017

Contributor

@minrk Do you think this ready to merge?

Contributor

gnestor commented Aug 4, 2017

@minrk Do you think this ready to merge?

@gnestor gnestor added this to the 5.1 milestone Aug 4, 2017

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 4, 2017

Member

@gnestor almost. There's one addition in check_and_sign that should be reverted, but the rest should be set.

Member

minrk commented Aug 4, 2017

@gnestor almost. There's one addition in check_and_sign that should be reverted, but the rest should be set.

@Madhu94

This comment has been minimized.

Show comment
Hide comment
@Madhu94

Madhu94 Aug 4, 2017

Contributor

@minrk The way store_signature for the signature store is implemented, the signatures don't get updated.
I logged an issue with nbformat, please let me know if I'm wrong here

Contributor

Madhu94 commented Aug 4, 2017

@minrk The way store_signature for the signature store is implemented, the signatures don't get updated.
I logged an issue with nbformat, please let me know if I'm wrong here

@Madhu94

This comment has been minimized.

Show comment
Hide comment
@Madhu94

Madhu94 Aug 4, 2017

Contributor

I think this is a bug even on master. I noticed that without the check, the same signature was re-inserted each time the notebook was trusted.

@minrk, I probably shouldn't check this here - the issue should be fixed in nbformat. So, I'll revert that change, squash my commits and then we're good to go?

Contributor

Madhu94 commented Aug 4, 2017

I think this is a bug even on master. I noticed that without the check, the same signature was re-inserted each time the notebook was trusted.

@minrk, I probably shouldn't check this here - the issue should be fixed in nbformat. So, I'll revert that change, squash my commits and then we're good to go?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 5, 2017

Member

@Madhu94 exactly right. This shouldn't be changed here, but fixed in nbformat itself. Thanks for digging in!

Member

minrk commented Aug 5, 2017

@Madhu94 exactly right. This shouldn't be changed here, but fixed in nbformat itself. Thanks for digging in!

@Madhu94

This comment has been minimized.

Show comment
Hide comment
@Madhu94

Madhu94 Aug 5, 2017

Contributor

@minrk I rebased and squashed my commits and reverted that check in manager.py. Let me know if there's anything else left?

Contributor

Madhu94 commented Aug 5, 2017

@minrk I rebased and squashed my commits and reverted that check in manager.py. Let me know if there's anything else left?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 7, 2017

Member

@minrk this is, I think, the last remaining PR for 5.1. Is it ready to go?

Member

takluyver commented Aug 7, 2017

@minrk this is, I think, the last remaining PR for 5.1. Is it ready to go?

@minrk minrk merged commit da2d54f into jupyter:master Aug 7, 2017

4 checks passed

codecov/patch 63.63% of diff hit (target 0%)
Details
codecov/project 79.71% (-0.03%) compared to fcc5b0f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 7, 2017

Member

Yes, indeed.

Member

minrk commented Aug 7, 2017

Yes, indeed.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 7, 2017

Member

Thanks, @Madhu94!

Member

minrk commented Aug 7, 2017

Thanks, @Madhu94!

@Madhu94 Madhu94 deleted the Madhu94:trust_without_save branch Aug 13, 2017

@Madhu94

This comment has been minimized.

Show comment
Hide comment
@Madhu94

Madhu94 Aug 18, 2017

Contributor

Can I add this to the 5.1 changelog ?

Contributor

Madhu94 commented Aug 18, 2017

Can I add this to the 5.1 changelog ?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 18, 2017

Member

Yes, please do!

Member

takluyver commented Aug 18, 2017

Yes, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment