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

[TASK] Switch Documentation Rendering to PHP #870

Merged
merged 3 commits into from
May 16, 2024
Merged

Conversation

Alagts
Copy link
Contributor

@Alagts Alagts commented May 15, 2024

You can try out the rendering locally with

make docs

You can try out the rendering locally with
```
make docs
```
Makefile Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add this with export-ignore to .gitattributes since it's not useful for Composer dist archives.

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 it still be helpful to have in vendor/? Composer users could easily locally create documentation if they wanted to? It's a bit different than a makefile that spawns tests only...?

Copy link
Contributor

Choose a reason for hiding this comment

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

For development, sure. (But the docs should then be outputted somewhere else besides vendor). But on production not really. You only want to upload the bare minimum of code necessary to have a running system here. Documentation and similar should be left out here. (And yes, this would mean that the whole Documentation directory should also be skipped besides a few others.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! Maybe then a separate PR to sort this out for the other involved components/files?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could make sense yeah. But for now we should avoid adding more dev-code to prod-archives.

Copy link
Contributor Author

@Alagts Alagts May 15, 2024

Choose a reason for hiding this comment

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

I added /Makefile export-ignore to .gitattributes.

Makefile Outdated
@@ -0,0 +1,14 @@
.PHONY: help
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Makefile only used for local rendering? Or is it also necessary for CI processes to work properly?

Currently, we don't have a Makefile in the project, so I would prefer to either migrate all commands to the Makefile or to move the docs commands to composer.json.

Choose a reason for hiding this comment

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

It is only used for the local rendering, however the issue with composer commands is, that you need php to run composer and many local machines run php in a docker container. Then composer is trying to execute docker in docker. Therefore I would not advise to add it to the composer.json. You could, however add the command to the runTests.sh if you want. You can also omit the Makefile and ppl can execute the docker or podman command directly. However we found that a Makefile is very helpful to a large number of contributors and we use it all over the official documentation. We also advise it to all Extensions. And also there is really no harm in having a Makefile. You might want to add shortcuts for the runTests.sh in future

@sbuerk
Copy link
Contributor

sbuerk commented May 16, 2024

We will discuss this dedicated how to proceed and how which way of dispatching we use in standalone fluid (runTests.sh approach / composer script approach / Makefile (with all).
Also adding a CONTRIBUTION.md file to describe these kind of things (test execution and so on).

Thus remove the Makefile from this PR for now and integrate the CI run for it first and directly and not postpone this change until decision of the topics above are made.

You can try out the rendering locally with
```
docker run --rm --pull always -v $(pwd):/project -it ghcr.io/typo3-documentation/render-guides:latest --config=Documentation
```
@Alagts
Copy link
Contributor Author

Alagts commented May 16, 2024

I removed the Makefile. Now, you can try out the rendering locally with

docker run --rm --pull always -v $(pwd):/project -it ghcr.io/typo3-documentation/render-guides:latest --config=Documentation

@s2b s2b merged commit 389ac38 into TYPO3:main May 16, 2024
4 checks passed
@s2b
Copy link
Contributor

s2b commented May 16, 2024

Thanks! :)

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.

6 participants