-
Notifications
You must be signed in to change notification settings - Fork 71
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
CASSANDRA-19915 Upgrade for the native protocols spec page #282
base: trunk
Are you sure you want to change the base?
Conversation
8391401
to
fc53c8c
Compare
# Variables | ||
GO_VERSION="1.20.6" | ||
GO_TAR="go${GO_VERSION}.linux-amd64.tar.gz" | ||
GITHUB_REPO_PARSER="https://github.com/martin-sucha/cqlprotodoc.git" # Replace with the actual parser repo URL |
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.
I'm hesitant to leave this pointing to a living branch. Pinning it to a SHA would be safest.
And am curious if we could upstream it to here… (but this can happen separately…)
@martin-sucha, would you be willing to upstream it (to either cassandra-website or cassandra-builds) ?
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.
Yes, it is better to copy it under github.com/apache
(I'll leave up to you to which repo). I'm willing to donate the cqlprotodoc code to the Apache software foundation and change the licence to Apache license, Version 2.0.
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.
Thanks @martin-sucha. That's all the approval/consent we need.
@tengu-alt , are you willing (and got the time) to include the contents of the repo in the folder cqlprotodoc
and adjust accordingly, in this PR ? (don't include its LICENSE file, add to its readme a sentence about where it comes from)
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.
Thanks @martin-sucha. That's all the approval/consent we need.
@tengu-alt , are you willing (and got the time) to include the contents of the repo in the folder
cqlprotodoc
and adjust accordingly, in this PR ? (don't include its LICENSE file, add to its readme a sentence about where it comes from)
Sure! I will do it.
This is a solid piece of work, and valuable addition to the website !! @ossarga , would you be interested in glancing over this in review, as it touches on your past work ? |
2bb65c0
to
3aba510
Compare
3aba510
to
c55a221
Compare
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. Thanks again @tengu-alt ! final approving depends on the other reviewers.
|
||
[source, js] | ||
++++ | ||
<script> |
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.
Since we moved the cqlprotodoc code here, I wonder if including the content using Javascript is still needed or whether we should modify the template.gohtml
/ cqlprotodoc output and include the html files directly.
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.
I think it's better to keep things as they are because the script placed in the .adoc template appears more readable to me. Also, we avoid changing the website template by keeping native_protocol.html as the location where the generated pages are merged.
Thanks a lot! Will wait for reviews :) |
c55a221
to
f1f5b4d
Compare
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.
Thanks! Looks good to me. I'll leave up to you whether to change to directly include the contents instead of using Javascript.
Thank you! |
going to merge this soon. |
@tengu-alt can you please tell me how you succeeded to render this locally? All I am getting is this: I checked out your branch and did file:///home/fermat/dev/cassandra/cassandra-website/site-content/build/html/_/native_protocol.html |
I have also run |
@smiklosovic you need to build an image from a |
@smiklosovic Has my advice helped solve the build issue? |
@tengu-alt I did what you suggested, it is still same. After your steps I just do
firefox opens that patch and I just have same results as shown in the comment from (1) (1) #282 (comment) |
@michaelsembwever does this render fine for you based on the steps @tengu-alt gave? I am a little bit out of options here. |
Because of the site-ui/ change in the PR, the way to build this should be
This is what's documented in this repo's README, and it is how ci-cassandra.apache.org builds the website (so it needs to work this way). This looks to work for me, at least i'm getting the (It would also be nice to turn off the progress reporting as go downloads…) |
@smiklosovic Now I see. There are some issues with |
@smiklosovic I've done some research, and it turns out that the browser does not show the content because the page is loaded using the |
[source, js] | ||
++++ | ||
<div id="contentv3" class="doc-container"></div> | ||
<iframe id="iframev3" src="native_protocol_v3.html" style="display:none;"></iframe> |
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.
is there a way to include the generated native_protocol_v3.html page, instead of using an iframe ?
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.
It can be done using the fetch
but it's relying on the same CORS
policy.
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.
can it not be included (somehow) at antora build time ? if possible that would be the best IMO.
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.
Could you please clarify - did you mean the JavaScript contained within the .adoc file?
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.
no. javascript and iframes are all doing the including at view time.
we should be embedding the html contents into the other file at antora build time.
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.
this is what i mean: c7b6b60
note, some small things are still needed on top of that^
(e.g. there's links in the headers of each page that don't work, and we need gitignore if we have to put the generated files under site-content)
(feel free to squash that commit into your work here)
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.
Understood. I'll try to move it into the .hbs
layout.
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.
Also… there's another thing we might want cleaned up here.
The native protocol spec files are actually here:
https://github.com/apache/cassandra/tree/trunk/doc
They're the source of truth, and those that will be kept up to date.
IUUC the ones in cassandra-website are unnecessary duplicates: https://github.com/apache/cassandra-website/tree/trunk/site-content/source/modules/ROOT/examples/TEXT
We can assume that these from apache/cassandra:trunk are accurate. And delete the examples/TEXT
directory. And then it might be possible then to not need any gitignore addition.
Going further, we could also remove the native_protocol.adoc file from casandra-website altogether, move it to here instead: https://github.com/apache/cassandra/tree/trunk/doc/modules/cassandra/pages
(this would mean running process-native-protocol-specs-in-docker.sh
on each generate_cassandra_versioned_docs()
)
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.
Sounds great! I'll apply it.
@smiklosovic I have to update my solution: for some reason the files that we need for the complete page render are not stored under the |
@tengu-alt I would prefer to wait until the complete / working solution is in place, if you do not mind. |
f1f5b4d
to
b141c20
Compare
@michaelsembwever I have moved the HTML and JavaScript from the The issue with the headers arises because the Could you also clarify why we need to move the |
The |
Please do, thanks!
The .spec files are originally found under https://github.com/apache/cassandra/tree/trunk/doc/ |
Done: apache/cassandra#3714 |
Understood, working on it. |
I've taken your latest work @tengu-alt and working it a bit. |
I am currently Implementing the protocol pages generation on each version(Maybe it will not be done exactly inside the |
Ah, i misunderstood you – didn't realise you were still in-progress… |
The |
Go for it. It's only for reference sake, and might not be used at all depending on what you land 👍 |
b141c20
to
cb3b785
Compare
@michaelsembwever I have done some refactoring on the The The (I also added the license part inside script) |
This PR enhances the Native Protocols page by replacing the plain text with an interactive, section-based navigation. The implementation leverages cqlprotodoc tool that converts CQL protocol specification files into HTML.
A
.sh
script was added to downloadcqlprotodoc,
convert the.spec
files containing the native protocol specifications into HTML pages, and then merge them into a single page.After that, native protocols page looks like this:
The navigation is clickable, so you can move to the desired section.