search

vericast / nbconflux

nbconflux converts Jupyter Notebooks to Atlassian Confluence pages
BSD 3-Clause "New" or "Revised" License
117 stars 33 forks source link

Hopefully fix issue #30 and #32 #31

Open danizen opened 4 years ago

danizen commented 4 years ago
tabbal commented 4 years ago

Thanks @danizen for putting this together. We are reviewing your changes. Maybe we can get @parente's thoughts too.

parente commented 4 years ago

The reason I originally picked URI instead of attachments, I believe was so that a version of a page in the Confluence history retained the explicit link to the version of the image that was embedded in the notebook at the time of the posting. With attachments, I believe a version of a page in the history would also update to point to the attachment on the latest version of the page rather than the attachment at the time the page was posted.

That behavior could have changed in newer Confluence versions. I no longer have access to a Confluence server to test any of the PRs here. @tabbal, you're the best candidate to review or maybe, @danizen, you can try to confirm or deny the above behavior.

tabbal commented 4 years ago

Thanks @parente. Hey @danizen, can you please share your tests results, including the Confluence version you're using?

danizen commented 4 years ago

@tabbal , the web links didn't work right in my copy of Confluence. If I go back to a version from the history where an image is not rendering properly, I see the problem. The web link is https://example.com/download/attachments/146342104/output_1_0.jpeg?version=5. A correct link in my environment would be https://example.com/confluence/download/attachments/146342104/output_1_0.jpeg?version=5. I will attempt tomorrow to figure out why this link is wrong.

Regardless of the reason; I think the design described by @parente does not meet the KISS principal, and I think it is brittle because many times a Confluence instance may be behind a reverse proxy. I also think that the ability to go back to an earlier published version from the Confluence history is not as important as the brittleness.

If you are not convinced of the brittleness, I can explain - I do not want to use techno babble about Apache httpd2 directives, Java containers, response header and content re-writing, etc.

My understanding of @parente's design is that he is trying to make sure that the right thing happens if a confluence user simply restores a previous version of a published notebook. A user might try this. Of course, a confluence user might not understand that the point is "notebook as code" and might edit the page anyway once getting it "into" confluence. So, there is no real way to keep things "in sync". My ideal nbconvert user will prefer to re-run nbconflux (i.e. using the notebook from another git branch), and even if they do not, the original notebook version is attached to the confluence page.

Enough said -- if you are convinced, great. If not, I'll figure out why the link is different, and try to fix that.

danizen commented 4 years ago

Answer for earlier questions: applinksVersion - 5.4.7 confluenceVersion - 6.13.4

I think that preserving the original behavior with a corrected ri:url is probably the better way to go. I see where to fix it near preprocessor.py:100, so I will work on a fixed pull request.

tabbal commented 4 years ago

Awesome, thanks @danizen!