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

Hopefully fix issue #30 and #32 #31

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Hopefully fix issue #30 and #32 #31

wants to merge 3 commits into from

Conversation

danizen
Copy link
Contributor

@danizen danizen commented Sep 5, 2019

Dan Davis added 3 commits September 5, 2019 15:32
- Since the image is attached to this page, using ri:attachment should be better than ri:url
- The documentation is at https://confluence.atlassian.com/doc/confluence-storage-format-790796544.html#ConfluenceStorageFormat-Images
  at the time of this writing.
@danizen danizen changed the title Hopefully fix issue #30 Hopefully fix issue #30 and #32 Sep 5, 2019
@tabbal
Copy link

tabbal commented Sep 17, 2019

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

@parente
Copy link
Contributor

parente commented Sep 17, 2019

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
Copy link

tabbal commented Sep 17, 2019

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

@danizen
Copy link
Contributor Author

danizen commented Sep 18, 2019

@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
Copy link
Contributor Author

danizen commented Sep 18, 2019

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
Copy link

tabbal commented Sep 19, 2019

Awesome, thanks @danizen!

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