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

Tweaking the Datasource guide content conditionals #43884

Merged

Conversation

MichalMaler
Copy link
Contributor

@MichalMaler MichalMaler commented Oct 15, 2024

This is the tweaking PR for the better reusability of the TLS guide content.
These changes are being made based on QE feedback. CC Georgii Troitskii.
The visualization of the Quarkus TLS guide remains unchanged.

The issue now is that in order to render something correctly in RHBQ, I have to include a snippet that exists only in the RHBQ repository. Since we follow an "Upstream first" policy, I need to mention this include in the Quarkus documentation as well, even though it's wrapped in an ifdef condition, ensuring it doesn't affect Quarkus documentation (since the attribute that triggers the condition and the snippet itself are defined only in product repo).

Unfortunately, the GitHub docs preview bot fails because it attempts to locate the snippet that doesn’t exist in Quarkus, even though it’s not required.

Caused by: java.nio.file.NoSuchFileException: target/asciidoc/sources/resources/snippets/snip-note-derby.adoc

In my opinion, some changes will need to be made in io.quarkus.docs.generation.AssembleDownstreamDocumentation, which was likely written by @gsmet , whom I am kindly asking for advice/opinion/workaround.
Fix could be something like: When it is all about loading a file from the resources/snippets/ folder, ignore it in the check.

This is not a blocker, but something that would be great to have.

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Oct 15, 2024

This comment has been minimized.

Signed-off-by: Michal Maléř <mmaler@redhat.com>
@MichalMaler MichalMaler force-pushed the Tweaking-the-Datasource-guide-conditionals branch from 05d5422 to 090917f Compare October 16, 2024 10:13

This comment has been minimized.

Signed-off-by: Michal Maléř <mmaler@redhat.com>

This comment has been minimized.

@MichalMaler
Copy link
Contributor Author

MichalMaler commented Oct 16, 2024

All tests and builds work fine, but the GitHub bot preview fails because it tries to load the mentioned snipped, which is not part of the Quarkus repo.

Caused by: java.nio.file.NoSuchFileException: target/asciidoc/sources/resources/snippets/snip-note-derby.adoc

Essentially, I need an exception for the file check that allows it to pass if the file is truly not meant to be used.

The file is not supposed to be used because the attribute triggering the condition isn't defined, -> the file isn't required -> The check should be OK since the file that isn't part of Quarkus is not needed and, thus doesn't restrict anything.

Here is, just for context, the usage of this snippet in product docs:
https://gitlab.cee.redhat.com/quarkus-documentation/quarkus/-/merge_requests/1911/diffs?commit_id=7e3adc9ce3e5c3803f68623a8b44d2d644a6228c

@yrodiere
Copy link
Member

Full stack trace:

2024-10-16T11:20:44.6660863Z [INFO] --- exec:3.1.0:exec (assemble-downstream-doc) @ quarkus-documentation ---
2024-10-16T11:20:45.3148075Z ##[error]Exception in thread "main" java.io.UncheckedIOException: An error occurred while generating the downstream tree
2024-10-16T11:20:45.3155859Z 	at io.quarkus.docs.generation.AssembleDownstreamDocumentation.main(AssembleDownstreamDocumentation.java:257)
2024-10-16T11:20:45.3157158Z Caused by: java.nio.file.NoSuchFileException: target/asciidoc/sources/resources/snippets/snip-note-derby.adoc
2024-10-16T11:20:45.3158065Z 	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
2024-10-16T11:20:45.3158841Z 	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
2024-10-16T11:20:45.3159583Z 	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
2024-10-16T11:20:45.3160397Z 	at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:218)
2024-10-16T11:20:45.3161116Z 	at java.base/java.nio.file.Files.newByteChannel(Files.java:380)
2024-10-16T11:20:45.3161668Z 	at java.base/java.nio.file.Files.newByteChannel(Files.java:432)
2024-10-16T11:20:45.3162377Z 	at java.base/java.nio.file.spi.FileSystemProvider.newInputStream(FileSystemProvider.java:422)
2024-10-16T11:20:45.3163084Z 	at java.base/java.nio.file.Files.newInputStream(Files.java:160)
2024-10-16T11:20:45.3163657Z 	at java.base/java.nio.file.Files.newBufferedReader(Files.java:2922)
2024-10-16T11:20:45.3164235Z 	at java.base/java.nio.file.Files.readAllLines(Files.java:3412)
2024-10-16T11:20:45.3165085Z 	at java.base/java.nio.file.Files.readAllLines(Files.java:3453)
2024-10-16T11:20:45.3165909Z 	at io.quarkus.docs.generation.AssembleDownstreamDocumentation.getFiles(AssembleDownstreamDocumentation.java:281)
2024-10-16T11:20:45.3167069Z 	at io.quarkus.docs.generation.AssembleDownstreamDocumentation.getFiles(AssembleDownstreamDocumentation.java:298)
2024-10-16T11:20:45.3168115Z 	at io.quarkus.docs.generation.AssembleDownstreamDocumentation.main(AssembleDownstreamDocumentation.java:172)

@yrodiere
Copy link
Member

So, the cause for the exception is io.quarkus.docs.generation.AssembleDownstreamDocumentation#getFiles not complying with ifdef/ifndef pre-processor instructions. Maybe that can be fixed, maybe not. Seems complicated, but what do I know.

That being said:

The issue now is that in order to render something correctly in RHBQ, I have to include a snippet that exists only in the RHBQ repository. Since we follow an "Upstream first" policy, I need to mention this include in the Quarkus documentation as well

I would say there is a contradiction between "we follow an upstream first policy" and "I have to include a snippet that exists only in the RHBQ repository". But again, not exactly for me to decide.

If it's only that though... can't you just include an empty snip-note-derby.adoc in the upstream repo?

@MichalMaler
Copy link
Contributor Author

@yrodiere Thank you for the feedback and opinion!
Yeah, that is also possible, but even if I would do that empty snipper include, we would need to:

  • create an attribute for that snippet path (since this file will be on different places (and I am not sure this will be working with older bccutil that builds product docs)
    or
  • add conversion of that snippet PATH into our downstreaming script (change path of the Quarkus location for the RHBQ location during the conversion so that the product version of this snipped is loaded.)

I suggested the above because I thought creating some exceptions would be easier, and I would not add an empty file to the Quarkus repo.

@MichalMaler
Copy link
Contributor Author

@yrodiere @gsmet
Would you like me to create an empty snippet, store it somewhere in the Quarkus docs folder, remove the conditionals completely, and keep just plain include?

  • include::{attribute-path}/snip-note-derby.adoc[]

If you don't like this solution:

  • Some tweaks to the downstreaming process need to be made
    or
  • We can close this and inform QE of the decision.

Signed-off-by: Michal Maléř <mmaler@redhat.com>
@MichalMaler
Copy link
Contributor Author

Or, we can use the proposal of my second commit. If we accept this, I would kindly ask @gsmet to perform some script magic and translate the following:
include::_includes/snip-note-derby.adoc[] -> include::resources/snippets/snip-note-derby.adoc[] ? :)

If tweaking our downstreaming process is "no-go" I can still apply attributes to substitute the differences in the file paths (but I want to minimize my impact on the Quarkus repo and adding things only if necessary).

Copy link

quarkus-bot bot commented Oct 18, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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.

Copy link

github-actions bot commented Oct 18, 2024

🙈 The PR is closed and the preview is expired.

@MichalMaler
Copy link
Contributor Author

I discussed this with @rolfedh , and it seems we can handle the translation independently with Rolfe's replacement Python script.
Rolfe, can you please add your MR to this thread so that @gsmet can see it, and if it is all good in Quarkus books, maybe I can squash+rebase+and ask for the merge so that QE can verify this guide for 3.15?

@rolfedh
Copy link
Contributor

rolfedh commented Oct 18, 2024

Sure thing, @MichalMaler. I've created a downstream MR to update our python script that performs simple replacements, here: https://gitlab.cee.redhat.com/quarkus-documentation/quarkus/-/merge_requests/1927

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

This is necessary as downstream extension status is different than the upstream one.

Copy link
Contributor

@rolfedh rolfedh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I can't say I'm excited about all these slowly becoming more and more present in our doc.

@gsmet gsmet merged commit acfcab4 into quarkusio:main Oct 18, 2024
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 18, 2024
@michalvavrik
Copy link
Member

I can't say I'm excited about all these slowly becoming more and more present in our doc.

@gsmet we are open to your proposals, we can follow up either here or in the meeting and try whatever you suggest.

@MichalMaler MichalMaler deleted the Tweaking-the-Datasource-guide-conditionals branch October 19, 2024 10:47
@MichalMaler
Copy link
Contributor Author

@gsmet Thank you for the merge. I am just trying to do as minimalistic changes to Quarkus docs as possible while keeping the rendered output intacted, but also ensure that reusability of this content is aligned with other policies and standards, too.
I added the 3.15 label to this PR as it is the last docs addition to 3.15 release.

I agree with @michalvavrik , if there is a better and more clean solution, I am for it.

@gtroitsk
Copy link

@gsmet please include this PR to the next 3.15 backport bulk PR
CC @rsvoboda

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
None yet
Development

Successfully merging this pull request may close these issues.

7 participants