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 build error caused by content in datasource guide #41651

Closed

Conversation

rolfedh
Copy link
Contributor

@rolfedh rolfedh commented Jul 3, 2024

In community main, line 550 in /asciidoc/datasource.adoc is:

<<quarkus-agroal_quarkus-datasource-jdbc-transactions,`quarkus.datasource[.optional name].jdbc.transactions`>> to `xa

In the syncrhonization PR, it causes the following preview build failure,

Transforming the AsciiDoc content to DocBook XML... �[31m�[1mOpening and ending tag mismatch: literal line 566 and link, line 566, column 66�[0m �[31m�[1mUnable to parse the AsciiDoc built DocBook XML�[0m

In this PR, the previously mentioned line 550 has been transformed into:

xref:quarkus-agroal_quarkus-datasource-jdbc-transactions[`quarkus.datasource[.optional name].jdbc.transactions`] to `xa`.

If you inspect line 556 in the master.adoc file for this PR, https://jenkins.dxp.redhat.com/job/CCS/job/ccs-mr-preview/80313/artifact/pantheon/configure-data-sources/build/en-US/master.xml/*view*/, that same line renders as follows:

 <link linkend="quarkus-agroal_quarkus-datasource-jdbc-transactions"><literal>quarkus.datasource[.optional name</link>.jdbc.transactions</literal>] to <literal>xa</literal>.</simpara> </listitem>

In other words, the right bracket in name] terminates the <link> element prematurely. Instead, the right bracket in </literal>] should close that <link> element.

I have confirmed that escaping the right bracket in name] solves the issue, as shown in the following example:

xref:quarkus-agroal_quarkus-datasource-jdbc-transactions[`quarkus.datasource[.optional name\].jdbc.transactions`] 

Copy link

quarkus-bot bot commented Jul 3, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with dot

This message is automatically generated by a bot.

@rolfedh rolfedh requested review from gsmet and MichalMaler July 3, 2024 03:41
@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 3, 2024

Cross reference: https://issues.redhat.com/browse/QDOCS-789

Copy link

quarkus-bot bot commented Jul 3, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit f38268f.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@MichalMaler
Copy link
Contributor

@rolfedh Ok. lets try that out. Thank you for the PR. :)

@MichalMaler
Copy link
Contributor

I guess this is not something generated, thus the escape should not ruin anything, but I am not 100% sure.
I asked @jmartisk and he thinks that the best option would be to have an option to run our downstreaming pipeline on this PR directly; before merging.

@jmartisk
Copy link
Contributor

jmartisk commented Jul 3, 2024

I looked at the (upstream) documentation built from this PR and the backslash visibly appears visibly in the HTML. Screenshot:

image

So this doesn't look entirely correct to me, is there another solution?

@gsmet gsmet changed the title Fix build error caused by content in datasource guide. Fix build error caused by content in datasource guide Jul 3, 2024
@jmartisk
Copy link
Contributor

jmartisk commented Jul 3, 2024

Would replacing the [] with either "" or <> or '' be reasonable? I think so, and perhaps it would fix the downstream build

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Jul 3, 2024
@MichalMaler MichalMaler self-requested a review July 3, 2024 14:05
@gsmet
Copy link
Member

gsmet commented Jul 3, 2024

I think it's because we should only escape the character when preparing the downstream doc.

@gsmet
Copy link
Member

gsmet commented Jul 3, 2024

@rolfedh do you confirm that escaping the ] fixes the issue with the downstream tooling and that you don't see it in the output? If so I will adjust the downstream script.

@MichalMaler
Copy link
Contributor

@gsmet Hello Guillame. If @rolfedh will not be 100% sure about the fix, can we merge this, test the pipeline, and then revert it?
My apologies, but I am not sure right now hot to test it properly. Also, we dont want to add more load on you if we are not 100% sure.

@jmartisk
Copy link
Contributor

jmartisk commented Jul 3, 2024

Turns out the docs team currently doesn't have an easy way to test it without merging it.
I can help them by creating another pipeline trigger that can manually run the downstreaming process from a custom branch (rather than main), but definitely not today (and I'm not 100% sure I can make it work tomorrow, and I'm off on Friday).

The "dirty" way of merging, running the downstream pipeline, and then reverting may be a workaround if this needs to be fixed quickly.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jul 3, 2024

@jmartisk Thank you, Jan. You are just great. I am on PTO since next few hours, but I will let @rolfedh know and we can progress if this is all good in @gsmet books.

@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 3, 2024

Hi, @gsmet.

Thanks for asking. Yes, I verified that escaping the square bracket in name] resolves the issue. I merged the synchronization pipeline PR and performed a downstream build using bccutils, the same toolchain that PV1 uses. After inspecting the XML and HTML outputs, I confirmed that the link element is correctly formed with that one change.

Please go ahead with your proposed solution and close this PR.

@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 3, 2024

Turns out the docs team currently doesn't have an easy way to test it without merging it.

Good point, @jmartisk.

I did some research and found a solution so writers don't have to perform "dirty" merges. If a writer encounters a build failure in a synchronization job PR, they can use these git commands or the GitHub CLI gh pr checkout command to check out the PR as a local branch. There, they can perform local builds to inspect the failure and test potential fixes. Once they verify the fix locally, they can submit that fix to the community main branch and repeat the synchronization pipeline job.

@gsmet gsmet closed this Jul 3, 2024
Copy link

github-actions bot commented Jul 3, 2024

🙈 The PR is closed and the preview is expired.

@gsmet
Copy link
Member

gsmet commented Jul 3, 2024

I have some local tests running atm so can't work on that but I'll have a look tomorrow morning.

@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 3, 2024

I have some local tests running atm so can't work on that but I'll have a look tomorrow morning.

Thank you, @gsmet!

@jmartisk
Copy link
Contributor

jmartisk commented Jul 4, 2024

Turns out the docs team currently doesn't have an easy way to test it without merging it.

Good point, @jmartisk.

I did some research and found a solution so writers don't have to perform "dirty" merges. If a writer encounters a build failure in a synchronization job PR, they can use these git commands or the GitHub CLI gh pr checkout command to check out the PR as a local branch. There, they can perform local builds to inspect the failure and test potential fixes. Once they verify the fix locally, they can submit that fix to the community main branch and repeat the synchronization pipeline job.

If that works for you then great. What I had in mind was the ability to make "dry runs" of the synchronization pipeline that would be based on a branch of a particular (upstream) PR, instead of a regular branch like main. Then you could see immediately that an upstream PR changes something that would break the synchronization job later, so you could reject the PR right away and prevent this from happening.

If you later decide that this would be useful to you, I am able to help (it would require parametrizing some additional stuff in the downstream scripts).

@MichalMaler
Copy link
Contributor

@jmartisk That would be great. Let me have a chat with Rolfe on Monday to decide about this and we will get back to you.
I will be happy to help in any way.
Thanks and kind regards. Cheers!

gsmet added a commit to gsmet/quarkus that referenced this pull request Jul 4, 2024
gsmet added a commit to gsmet/quarkus that referenced this pull request Jul 4, 2024
@rolfedh
Copy link
Contributor Author

rolfedh commented Jul 8, 2024

@MichalMaler @jmartisk I invited Mickey to discuss this in about an hour or so.

gsmet added a commit to gsmet/quarkus that referenced this pull request Jul 9, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this pull request Jul 31, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation
Projects
Development

Successfully merging this pull request may close these issues.

4 participants