Implement REPLACE_ANCESTOR publish strategy

[laurentverbruggen]

In some cases you might want to keep siblings in your ancestor instead of cleaning everything up.
I made the default so that there are no breaking changes.

[coveralls]

Coverage increased (+0.4%) to 86.705% when pulling 32dc3ef on laurentverbruggen:feature/cleanup-ancestor into 8a6e593 on alainsahli:master.

[laurentverbruggen]

Implementation of issue #104

[laurentverbruggen]

@alainsahli can you verify this PR? Thanks

[laurentverbruggen]

@cstettler maybe you can verify the PR? Thanks

[cstettler]

hi @laurentverbruggen, sorry for the delay. Before discussing the actual code changes, I'd like to better understand your use case. In #76 we are also discussing adding support for different publishing / clean-up strategies, but were so far reluctant to implement them as the use cases and expected behaviour was not yet clear enough to us.

Your case seems to correspond to the second bullet point in #76, is this correct? If so, what would be the expected behaviour if you would rename the root page of one of your "sub system" documentation? Without any additional magic, the old root page (potentially including some child pages) would still be around after publishing the new documentation.

Any additional input from your "real world" scenario would be helpful, thanks!

[laurentverbruggen]

Well my use case is pretty straight forward. We already have a lot of documentation in a Confluence space (manually included, not managed by Git/Asciidoc) and I want to add (technical) documentation from a Git repository as a child page to one of the existing page in the space. For now, we created a child page in the ancestor page where the documentation should be included and use that child page as ancestor when using the publisher maven plugin.

This extra level is required to avoid cleanup of other pages that also exist under the intended ancestor.
Since I only have the single root page this extra level seems especially silly.

In the end it really is what you describe in your second bullet point in #76. I didn't notice that you already had an issue describing the problem. (although with a slightly different scenario)

[cstettler]

Thank you @laurentverbruggen for your input. Based on that, my understanding is that your use case seems to map better onto the first one described by #76, as you would like to get rid of the intermediary page.

Would that be an option in your case (i.e. you would specify the id of intermediary page as the ancestor id, but then replace the title and text of that intermediary page with your single top-level AsciiDoc page)? That scenario would not suffer from the issue of "orphaned" pages when renaming the top-level AsciiDoc page.

If not, what would be the expected behaviour in case you would rename to top-level AsciiDoc page (or more, its title)?

[laurentverbruggen]

@cstettler Yes, renaming the ancestor id page would also be fine as long as the siblings of the provided ancestor id are not cleaned up. I don't want to lose those since they are not generated.

[cstettler]

Thanks for the additional input @laurentverbruggen - in this case, maybe a configurable "publishing strategy" might indeed be the way to go, with (currently) two options: APPEND_TO_ANCESTOR vs. REPLACE_ANCESTOR (names debatable):

• APPEND_TO_ANCESTOR would append the published pages to the ancestor (while cleaning up all other existing pages under the ancestor) (current behaviour, default)

• REPLACE_ANCESTOR would replace the title and the content of the ancestor with the single root page (that would represent an additional constraint on the source documentation structure), and would also clean up all pages under that ancestor (but not touch any siblings of the ancestor)

Does that sound reasonable?

[laurentverbruggen]

Yep, sounds reasonable to me!

[laurentverbruggen]

@cstettler I was thinking about it and what if I wouldn't have the single root. So assume the following scenario. I have two pages that I want to add under an ancestor, but the ancestor already has different pages (manually created) that I want to keep. In that case both APPEND_TO_ANCESTOR and REPLACE_ANCESTOR are not applicable since the first will remove my manually created pages and the second will not work because it can only make this work for a single root.

[cstettler]

Yes, @laurentverbruggen, you are right, that's what I meant with

that would represent an additional constraint on the source documentation structure

in my last comment above. If we would go this way, the corresponding strategy would have to validate the source documentation structure and raise a meaningful exception, if more than one root page is present. I think that is not uncommon, AsciiDoc e.g. also disallows multiple top-level titles depending on the output document type. One might argue that the strategy REPLACE_ANCESTOR is probably only used (and useful) in the context of a source documentation structure with only one root.

In your example, what behaviour would you expect? Not only for the initial publication, but also, if the title of one of the top-level pages changes? Without any "magic" applied, you would end up with stale siblings.

[laurentverbruggen]

Ok yes @cstettler, I guess that makes sense. There is no real value in my last scenario because any changes to the page titles would result in 'disconnected' pages that you would need to cleanup manually. Ok, I'll give the two strategies a go and update this PR.

[laurentverbruggen]

Included ancestorId because I needed it for the replace strategy.

[cstettler]

I again analysed the need for reading back the ancestorId and passing in when updating the page:

• For all non-root pages, the ancestor id is defined by the content id of the parent page, so there is no need to use the ancestor id read from the existing page.

• When using the APPEND_TO_ANCESTOR publish strategy, the ancestor id of the root page is defined by the configuration.

• The only case the ancestor id from the existing page is currently used, is when using the REPLACE_ANCESTOR. But in this case, the hierarchy of the ancestor page should not be changed, so the ancestor id parameter does not even have to be passed to the confluence api, so null could be passed in this case. Unfortunately, currently, the HttpRequestFactory.updatePageRequest() method expects the ancestorId parameter to be present, which is no longer true in all situations.

Therefore, I would opt for making the ancestorId parameter in HttpRequestFactory.updatePageRequest() optional, not passing it to the PagePayload via the builder. That way, the ancestorId does not need to be read back from existing pages (and no magic handling for dealing with potential multiple ancestor is needed).

[laurentverbruggen]

@cstettler can you review it? Thanks

[znerd]

Would be very nice if this could get merged.

@cstettler Is this on your radar?

[cstettler]

It appears counter-intuitive to me that a page in a hierarchy might have more than one ancestor - do you know any scenario in which that might me the case?

[cstettler]

Would be very nice if this could get merged.

@cstettler Is this on your radar?

Hi @znerd, yes, I have since long, but unfortunately never found time to have a thorough look at the changes - until tonight ;-)

@laurentverbruggen I allowed myself to comment on your code changes with inline comments. In case you prefer me to directly do the changes, feel free to allow me to commit back to your pull request. Thank you for your contribution and your patience 😊

[cstettler]

Hi @laurentverbruggen, do you still plan to invest into this pull request (i.e. work on the review findings)? If not, we would like to cherry-pick your work and drive further the integration into the master.

[garretfick]

I would very much like to see this merged and could help fix things if this is stuck. I think my use case is different than the ones mentioned in #76, but think this is a good starting point.

In my case, we have a number of projects and would like to setup a single space on confluence to host published documentation. We push out new releases every ~2 weeks, and often our customers do not upgrade, so I would like to keep the old versions of the documentation available.

From that perspective, I would like to be able to do the following:

ancentorId (per project) -> (version #) -> docs

So I would like the replace strategy, but where we only replace within a child ancestor, matching based on some attribute.

To me, this sounds like a different publish strategy, so having this merged would help a lot in making that possible (and convincing the owners here that this is valuable).

[cstettler]

Hi @garretfick, thank you for your comment. I fully agree, we should bring this PR to the master asap. Due to the missing feedback of the original author, I will take care of fixing the review issues and merge the PR to the master within the next few days.

Regarding your use case: I think that is a very valuable one indeed. Currently, you could achieve that only by specifying a (manually created) new ancestor per version and project. With some external tooling you could automate this (e.g. by creating a new ancestor using the REST API and then configuring the Confluence Publisher to use that newly created ancestor), but having this support as part of the Confluence Publisher would be a lot nicer.

In any case, due to Confluence's requirement to have unique page titles, you would still have to add a version-based prefix or suffix to each page for each version (which is already supported by the Confluence Publisher).

Would you mind creating a separate issue for your use case so we can continue the discussion there? Thank you!

[cstettler]

Merging PR to pr/laurentverbruggen/feature/cleanup-ancestor for finalization and integration into master

[garretfick]

@cstettler Sorry I didn't see the mention (even though I'm subscribed)! I also now see the other comment in #122 so I can take a look at this when I get a break.

Thanks for merging it (even if it wasn't my code)