-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
@MichalMaler for now, I marked it for 3.7 only but I could be convinced to backport it to 3.2 if needed. |
🙈 The PR is closed and the preview is expired. |
@gsmet Hey! This is awesome! |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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.
// Allow only letters, -, _, . | ||
string = string.replaceAll("[^\\w-_\\.]", "-").replaceAll("-{2,}", "-"); | ||
// Allow only letters, -, _ | ||
string = string.replaceAll("[^\\w-_]", "-").replaceAll("-{2,}", "-"); |
There was a problem hiding this comment.
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?
This comment has been minimized.
This comment has been minimized.
@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. |
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). |
@yrodiere |
@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:
Many thanks. |
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. |
9a08db6
to
101e391
Compare
51efab6
to
a8ebc2d
Compare
This comment has been minimized.
This comment has been minimized.
a8ebc2d
to
dd76545
Compare
I think I went through all the links pointing to generated config doc...
dd76545
to
67ec29a
Compare
Failing Jobs - Building 67ec29a
Full information is available in the Build summary check run. 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✖ 📦 integration-tests/elasticsearch-rest-client✖ 📦 integration-tests/hibernate-search-orm-elasticsearch-tenancy✖ 📦 integration-tests/hibernate-search-orm-opensearch✖
📦 integration-tests/logging-gelf✖ ⚙️ Maven Tests - JDK 17 #- Failing: integration-tests/maven
📦 integration-tests/maven✖
✖
|
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. |
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