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

Enhancement/convert asciidoc #288

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

derochs
Copy link
Collaborator

@derochs derochs commented Jun 21, 2021

This PR includes changes that convert AsciiDoc syntax into Markdown syntax at these locations:

  • "text" and "textAfter' placeholders in renderTemplate will be converted into Markdown, as they may contain AsciiDoc syntax
  • in the destroy function, the playbook description will be converted into Markdown, as well as the playbook conclusion

Note: If this PR is merged, you'll have to install Pandoc and Asciidoctor on your local machine

Linux:

  • Pandoc: sudo wget https://github.com/jgm/pandoc/releases/download/2.14.0.2/pandoc-2.14.0.2-1-amd64.deb && sudo dpkg -i pandoc-2.14.0.2-1-amd64.deb
  • Asciidoctor: sudo apt install asciidoctor

Windows:

  • Pandoc:
    • First install chocolatey with PowerShell: Set-ExecutionPolicy Bypass -Scope Process -Force; [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.ServicePointManager]::SecurityProtocol -bor 3072; iex ((New-Object System.Net.WebClient).DownloadString('https://chocolatey.org/install.ps1'))
    • Then install pandoc: choco install pandoc
  • Asciidoctor:
    • First install Ruby: choco install ruby
    • Then install Asciidoctor: gem install asciidoctor

@GuentherJulian
Copy link
Contributor

Is there a way to check if Pandoc and Asciidoctor are installed and only do the conversion if they are. Then it is not really necessary to install the tools if you only run it locally for testing. Or maybe add a new parameter to skip the conversion.

@derochs
Copy link
Collaborator Author

derochs commented Jul 1, 2021

Makes sense, will add this feature.

this.createTempFile("temp.adoc", asciidocString);

let markdownString = "";
if(this.platform == ConsolePlatform.WINDOWS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this if-else-block is needed? Both block execute the same commands

this.executeCommand("pandoc -f docbook -t gfm temp.xml -o temp.md", this.getTempDir());
markdownString = this.readTempFile("temp.md");
}else {
markdownString = this.readTempFile("temp.adoc");
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you use markdownString = asciidocString in this case? Then you do not need to call the function


let markdownString = "";
if(this.platform == ConsolePlatform.WINDOWS) {
if(this.commandAvailable("asciidoctor") && this.commandAvailable("pandoc")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to test if both commands are available in the constructor once. Otherwise you will always test and execute the commands when you want to convert something

readTempFile(filename: string): string {
let file_path = path.join(this.getTempDir(), filename);
let fileContent = "";
let file = fs.readFileSync(file_path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you pass the encoding parameter you dont have to convert it to a string afterwards.

return process.status == 0;
}

executeCommand(command: string, directory: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the function executeCommandSync() we already got in ConsoleUtils for this ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically yes, but the executeCommandSync() from ConsoleUtils requires parameteres that I cannot provide (e.g. RunResult, env)

}
if("textAfter" in variables) {
variables["textAfter"] = this.converter.convertAsciidocToMarkdown(variables["textAfter"]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe also check for playbook.description and convert it to markdown. Now we don´t have a problem with that but somebody could also use asciidoc formating in the description in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I implemented it correctly, the description conversion can be found here

@EduardKrieger EduardKrieger self-requested a review July 15, 2021 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants