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

Fix confluence export #191

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zmully
Copy link

@zmully zmully commented Mar 11, 2022

An attempt at fixing some of the problems with the psp builder Confluence exporter:

  1. The existing relative link processing replaces all links on a page with the first link it finds. This is most obvious on the index.md page which links to all other pages.
  2. The existing relative link processing stripped all links of any relative page anchor ids (i.e data-mgmt.md#data-classification-model was stripped to just data-mgmt.md) because of the existing broken regex.
  3. There was a hack around GFM indentation for lists, but it only handled lists with one sublevel. Enabling the showdown option disableForced4SpacesIndentedSublists: true instead is preferable.

Problems I tried to fix, but couldn't:

  1. The template anchor ids, like [Data Classification Model](data-mgmt.md#data-classification-model) don't work in Confluence, as Confluence doesn't support them on import (? I'm not actually sure what is happening). Confluence instead generates it's own case-sensitive ids, #Data-Classification-Model, so deep linking doesn't work.
  2. The base templates do not follow a standard capitalization anchor id format, there are some Pascal-ish kebab (Lorem-Ipsum-Lorem), and some straight kebab (lorem-ipsum-lorem). I prefer the templates adhere to and enforce straight kebab if the Confluence import issues can be handled, so I don't have to bother with the correct captialization of the anchor ids.

@zmully
Copy link
Author

zmully commented May 9, 2022

Finally got some time to add support for exporting policies under a parent page and added some debugging since the export goes through a couple of intermediary steps that make it difficult to diagnose format issues in Confluence without.

There were no preexisting tests for any of the confluence code, so I didn't go there either with this PR. Additionally, I did not try to diagnose the test failures with existing code that I did not modify as it is unclear if those were related to my local setup, or if they've been broken for while.

@zmully zmully marked this pull request as ready for review May 9, 2022 17:41
@zmully zmully requested a review from a team as a code owner May 9, 2022 17:41
@zmully zmully requested review from a team May 9, 2022 17:41
@behobu behobu changed the base branch from main to test-fixConfluenceExport July 11, 2022 14:08
behobu
behobu previously approved these changes Jul 11, 2022
Copy link
Contributor

@behobu behobu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, merging into a test branch to test locally before merging into main

@behobu behobu changed the base branch from test-fixConfluenceExport to main July 18, 2022 19:36
@behobu behobu dismissed their stale review July 18, 2022 19:36

The base branch was changed.

Copy link
Contributor

@behobu behobu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug flag isn't being parsed out of the options, and the publish to confluence function doesn't seem to find the correct files to publish. I won't be merging this into the main branch for now.

@zmully
Copy link
Author

zmully commented Sep 7, 2022

and the publish to confluence function doesn't seem to find the correct files to publish.

@behobu can you please be more specific? I think this might have sat so long that you've since switched CI systems, and I can't see the results from around the time of your comment.

@zmully zmully force-pushed the fixConfluenceExport branch from 57ab596 to fb139c2 Compare September 14, 2022 18:53
@zmully
Copy link
Author

zmully commented Dec 2, 2022

👋 Sorry to do this, but I'd love to get some attention from J1 staff on this PR. It looks like behobu may no longer be an employee of J1, so I'm unfortunately going to spam active J1 people for attention.

Anyone? Anyone? Bueller? Bueller?
@philidem @aiwilliams @zemberdotnet @austinkelleher

Additionally, in this last commit, I fix the broken revision logic. It looks like you had some copy pasta to provide a defaultRevision if none were specified, but one overwrote the config.json value and the other didn't. I've removed the duplicate function and fixed the logic for the revision.

@zemberdotnet
Copy link
Member

zemberdotnet commented Dec 2, 2022

Hey @zmully 👋 Thanks for the PR! @behobu is still at J1. He should be able to review the PR.

One thing, do you mind backing out the changes to yarn.lock. We typically update dependencies internally for security reasons.

@zmully zmully requested a review from behobu December 6, 2022 16:54
@aiwilliams
Copy link
Contributor

Hello @zmully. I just checked in on the status of getting this reviewed and merged. It hasn't been forgotten, but the expectation is that it may take a bit of time to validate fully, so it's on the list but not at the top. Thank you for your contribution and patience!

@zmully
Copy link
Author

zmully commented Mar 27, 2023

@aiwilliams we're coming up on a year and change, and I'm having to use my own branch again to publish my policy docs internally to our wiki. I can't imagine I'm the only one who is stuck publishing their policies to Confluence, and I'd appreciate it if either you all could review this PR, or if you've no interest the PR, then please let me know, so I can close it, and maintain my own patched version of the tool.

@SeaBlooms SeaBlooms requested review from SeaBlooms and removed request for a team November 16, 2023 16:03
@zmully zmully requested a review from a team as a code owner January 22, 2024 21:41
@SeaBlooms
Copy link

@zmully reviewing this

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.

5 participants