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

Avoid dots in config doc ids as it's causing issues for downstream #38430

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Jan 26, 2024

I think I went through all the links pointing to generated config doc...

/cc @quarkusio/documentation
/cc @yrodiere you might want to check I didn't break anything in your doc, even if I tried to be thorough and use cross references to enable the checks

@quarkus-bot quarkus-bot bot added area/core area/docstyle issues related for manual docstyle review area/documentation labels Jan 26, 2024
@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Jan 26, 2024
@gsmet
Copy link
Member Author

gsmet commented Jan 26, 2024

@MichalMaler for now, I marked it for 3.7 only but I could be convinced to backport it to 3.2 if needed.

Copy link

github-actions bot commented Jan 26, 2024

🙈 The PR is closed and the preview is expired.

@MichalMaler
Copy link
Contributor

@gsmet Hey! This is awesome!
Thank you so much!
Will test it on Monday. Yes, we will need this on 3.2, just for our testing and for others to get familiar with the PV1 publishing process and debugging.

@quarkus-bot

This comment has been minimized.

@quarkus-bot quarkus-bot bot added the area/hibernate-search Hibernate Search / Elasticsearch label Jan 29, 2024
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

LGTM apart from a few oversights, see suggestions below.

Also, while I'm absolutely not blocking this PR for that reason, I don't like that pretty much all links to the Quarkus documentation that I've posted on the web so far will get broken because of this. IMO whatever bug in downstream tools led to this PR should be fixed in downstream tools instead.

Comment on lines -56 to +57
// Allow only letters, -, _, .
string = string.replaceAll("[^\\w-_\\.]", "-").replaceAll("-{2,}", "-");
// Allow only letters, -, _
string = string.replaceAll("[^\\w-_]", "-").replaceAll("-{2,}", "-");
Copy link
Member

Choose a reason for hiding this comment

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

So we're breaking every single link pointing to Quarkus configuration properties from other websites? I left quite a few of those out there, on stackoverflow, Zulip, Discourse, GitHub, ...

Can we add a secondary anchor for the deprecated, dot-containing id?

Quarkus Documentation automation moved this from To do to Review pending Jan 29, 2024
@quarkus-bot

This comment has been minimized.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jan 29, 2024

@yrodiere Hello Yoann,

I think you at least deserve to know why this is happening. RHBQ was stuck for a long time on the old and unsupported publishing platform Pantheon II. We implemented a few workarounds to make publishing possible, yet operating on that tool was painful. Also, we were the last team there; the others had already fled to something else.

While the migration to the older, but technically savvier brother, Pantheon 1, offers many benefits and solves hundreds of small issues, it causes the one we are resolving right now. I asked the CSS tool team and bccutil gurus, but I was told that a rule that makes reading these IDs problematic cant be bypassed and rule can not be shuted down because of security reasons. I did what I could but needed to turn to upstream masterminds again for a help.

I asked Guillaume if the transformation of these links and IDs could be part of our script, but he proposed this approach and unification. I am sorry to hear that it causes some problems, but once this is resolved, we should be in a much better position and operate on an overall better publishing tool.

On behalf of the entire docs team, I want to apologize for any additional workload and problem-solving these changes brings.

@yrodiere
Copy link
Member

yrodiere commented Jan 29, 2024

I understand, Michal. Thanks for the details.

I'd simply suggest that moving forward, the docs team considers the impact of breaking links. And I don't mean just in this specific case: I suspect we've had several similar changes in the past, like renaming a guide, or simply changing headings that relied on auto-generated anchors.

It's not that it causes additional work for upstream, it's more that it drastically reduces the amount of time that that work will remain useful. We have lots of discussions and questions/answers referencing upstream docs out there, and every time we make a change like this, we make a few of those discussions obsolete, by preventing people from following relevant links. That's basically getting rid of part of the "documentation ecosystem", one piece at a time.

I understand we can't keep compatibility forever, but I think that problem needs to at least be considered, and mitigated where possible (which is not here, from what I understand).

@MichalMaler
Copy link
Contributor

@yrodiere
The changes we need can be part of the script we are already using, so that Upstream content remains the same, and RHBQ repo will get what it needs. I am just not good in programming to propose something that could be applicable.
I sent you an email with our bccutil's problem description, just so you have all the info you would need to make a call.

@MichalMaler
Copy link
Contributor

MichalMaler commented Jan 29, 2024

@gsmet Hello Guillaume,

A backport to 3.2 would be great, but it's not essential. If it's not a straightforward merge and requires extra work, please feel free to drop the idea. I don't want to take up your time on something that isn't absolutely necessary, unlike the ID transformation.

If it's an easy task, I'd appreciate it.

As I mentioned in our email conversation, we're comfortable with either solution:

  1. Applying this transformation only to our RHBQ set of files during single-sourcing.
  2. Or the direct patch as you're suggesting here.

Many thanks.

@gsmet
Copy link
Member Author

gsmet commented Jan 29, 2024

The changes we need can be part of the script we are already using, so that Upstream content remains the same

Unfortunately, it's not easy to identify these links in a reliable way, thus why I did the change for upstream too.

It's something we will have to live with.

@quarkus-bot

This comment has been minimized.

Quarkus Documentation automation moved this from Review pending to Reviewer approved Jan 30, 2024
I think I went through all the links pointing to generated config doc...
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 30, 2024

Failing Jobs - Building 67ec29a

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 🔍
JVM Tests - JDK 21 Build Failures Logs Raw logs 🔍
Maven Tests - JDK 17 Build Failures Logs Raw logs 🔍
✔️ Maven Tests - JDK 17 Windows 🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: integration-tests/elasticsearch-java-client integration-tests/elasticsearch-rest-client integration-tests/hibernate-search-orm-elasticsearch-tenancy and 2 more

📦 integration-tests/elasticsearch-java-client

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-elasticsearch-java-client: I/O Error

📦 integration-tests/elasticsearch-rest-client

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-elasticsearch-rest-client: I/O Error

📦 integration-tests/hibernate-search-orm-elasticsearch-tenancy

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-hibernate-search-orm-elasticsearch-tenancy: I/O Error

📦 integration-tests/hibernate-search-orm-opensearch

io.quarkus.it.hibernate.search.orm.opensearch.devservices.HibernateSearchOpenSearchDevServicesEnabledImplicitlyTest.testDevServicesProperties - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.elasticsearch.restclient.common.deployment.DevServicesElasticsearchProcessor#startElasticsearchDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/opensearchproject/opensearch:2.9.0
	at io.quarkus.elasticsearch.restclient.common.deployment.DevServicesElasticsearchProcessor.startElasticsearchDevService(DevServicesElasticsearchProcessor.java:108)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:849)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)

📦 integration-tests/logging-gelf

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-logging-gelf: I/O Error


⚙️ Maven Tests - JDK 17 #

- Failing: integration-tests/maven 

📦 integration-tests/maven

io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 967 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "d02b2b8b-7a84-47d0-9ead-b19bf1bcc983" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AbstractHamcrestCondition.await(AbstractHamcrestCondition.java:86)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:691)
	at io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed(DevMojoIT.java:967)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 967 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "d02b2b8b-7a84-47d0-9ead-b19bf1bcc983" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AbstractHamcrestCondition.await(AbstractHamcrestCondition.java:86)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:691)
	at io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed(DevMojoIT.java:967)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

@gsmet gsmet merged commit 9c56d92 into quarkusio:main Jan 30, 2024
52 of 54 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Jan 30, 2024
@gsmet gsmet added this to the 3.7.1 milestone Jan 30, 2024
@gsmet gsmet modified the milestones: 3.7.1, 3.2.11.Final Jan 31, 2024
@ppalaga
Copy link
Contributor

ppalaga commented Jan 31, 2024

Ouch, this hits also Camel Quarkus and Quarkus CXF. We can invest the work in our current branches, but pretty please do not backport this to 3.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/docstyle issues related for manual docstyle review area/documentation area/hibernate-search Hibernate Search / Elasticsearch
Development

Successfully merging this pull request may close these issues.

None yet

4 participants