-
Notifications
You must be signed in to change notification settings - Fork 95
AV-55075 Update playbook and Jenkins for @asciidoctor/tabs (Prod) #694
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
Conversation
|
This is for Prod. |
| "@antora/cli": "~3.0", | ||
| "@antora/site-generator-default": "~3.0", | ||
| "@antora/site-generator-ms": "git+https://gitlab.com/opendevise/oss/antora-site-generator-ms#as-extension", | ||
| "@asciidoctor/tabs": "^1.0.0-beta.5", |
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 guess my only thing - are we cool with implementing something that's still in beta and have we tested appropriately?
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.
Good question! @mojavelinux what remains for you to call this a 1.0.0 release?
Happy to put some resources into testing with a specific plan against our estate.
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.
To be honest, I was kind of waiting for Couchbase to migrate to Asciidoctor Tabs as that could tease out any bugs that may have been introduced by setting up the project. If you can verify that it's ready to go, that would definitely give me the verification I need to call it 1.0.0.
Keep in mind that beta means "no breaking changes, only bug fixes and new features". So a beta is very stable. It's only really an alpha I would question. But still, we might as well get it over the line.
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 @mojavelinux, I wondered if that might be the case!
From my POV, the docs were very clear and smooth. The main things I noted in the upgrade process were:
- the preview extension loading, which you submitted the fix for (but that's just tech debt on our part)
- overriding the CSS. (I ended up deleting all our existing CSS, and copying/customizing the bundled
@asciidoctor/tabs.cssif you have any comments on AV-55075 Tabset upgrade docs-ui#165 do shout). But again, I imagine most/all users will be incorporating this fresh, rather than upgrading from the original script. - the repo README.adoc should link directly to the excellent Antora page.
@sarahlwelton can you have a quick look at your sections for any issues? (and let me know your thoughts on what additional testing will reassure you best)
antora-playbook.yml
Outdated
| idprefix: '@' | ||
| idseparator: '-@' | ||
| tabs: tabs | ||
| tabs-sync-option: true |
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.
The correct way to turn on an attribute (in this case global option) is an empty string, not the value "true". In other words:
tabs-sync-option: ''
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.
Ah, I've seen that, but as an empty string "looks" falsey, and it looks like any defined value counts as truthy, this felt more explicit?
Is the fact that true works undefined behaviour, and therefore
likely to change in future?
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 may look weird, but it is how AsciiDoc has always been. The reason for it is that in a document, we can't rely on types like true and false. (The value true would just mean the attribute has the string value "true"). So it's better to be consistent with what is available there. It's the equivalent to:
:tabs-sync-option:
In the AsciiDoc Language project, we may eventually define a true value as a way to officially set and attribute (to an empty value)...but we haven't crossed that bridge. However, we can be certain that an empty value will always set the attribute.
package.json
Outdated
| "@antora/cli": "~3.0", | ||
| "@antora/site-generator-default": "~3.0", | ||
| "@antora/site-generator-ms": "git+https://gitlab.com/opendevise/oss/antora-site-generator-ms#as-extension", | ||
| "@asciidoctor/tabs": "^1.0.0-beta.5", |
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 would be best to depend on the exact version whenever using a prerelease. So 1.0.0-beta-5 (no leading caret).
|
Tab sync is looking good so far on Staging. Once merged, we can keep an eye on it in Prod and report back to @mojavelinux with any issues before flicking switch to stable 1.0 |
Requires couchbase/docs-ui#165