Codeberg/Documentation
42
199
Fork
You've already forked Documentation
161

Add documentation for Forgejo CI Actions runner #369

Merged
n0toose merged 3 commits from :ci/actions into main 2023年12月07日 17:58:10 +01:00
Contributor
Copy link

This should solve #318.

This should solve #318.

@linusg Hey, just to be sure, it is OK for us / the PR author to reuse material from your blog (incl. screenshots) under the terms of the license of this repository (with attribution in the commit information and a small thank you note), correct? We are not sure how much of it we'll use yet, but it won't be substantial.

(We would also very much appreciate your input on this PR!)

@linusg Hey, just to be sure, it is OK for us / the PR author to reuse material from your blog (incl. screenshots) under the terms of the license of this repository (with attribution in the commit information and a small thank you note), correct? We are not sure how much of it we'll use yet, but it won't be substantial. (We would also very much appreciate your input on this PR!)
First-time contributor
Copy link

Yes, absolutely fine :)
Just keep in mind that a few things have changed since I wrote that (at least I had to tweak my hosted one at least once), so some information may be outdated. I'll take a proper look at this PR later 👍

Yes, absolutely fine :) Just keep in mind that a few things have changed since I wrote that (at least I had to tweak my hosted one at least once), so some information may be outdated. I'll take a proper look at this PR later 👍
n0toose left a comment
Copy link

This is a great PR, and especially a great first PR.

I didn't execute the instructions to confirm their accuracy because of my lack of experience in that sector, which is super important for a "step-by-step"-esque tutorial. There are also remaining TODOs and I would appreciate a second opinion; feel free to contact me again and I'll try to make the time to take a much more careful look if this does not happen soon after you complete those TODOs.

But language- and style-wise, everything is pretty great, proven by the fact that I can't really go beyond "grasping at straws" in a document of this size - and, apart from one style issue, the only "actual" problems are purely cosmetic does not concern instructions, as in, prevent the user from doing what they need to do. I would even merge it as it is, as this goes beyond the standards of what we normally expect.

As this is the perspective I normally review PRs from, I am submitting a preliminary approval, even if this is not 100% ready to merge. If everything else (accuracy of instructions, screenshot) are good, I'm fine with merging this as-is.

Overall, great job, and thank you.

This is a great PR, and especially a great first PR. I didn't execute the instructions to confirm their accuracy because of my lack of experience in that sector, which is super important for a "step-by-step"-esque tutorial. **There are also remaining TODOs and I would appreciate a second opinion; feel free to contact me again and I'll try to make the time to take a much more careful look if this does not happen soon after you complete those TODOs.** But language- and style-wise, everything is pretty great, proven by the fact that I can't really go beyond "grasping at straws" in a document of this size - and, *apart from one style issue*, the only "actual" problems are purely cosmetic does not concern instructions, as in, prevent the user from doing what they need to do. I would even merge it as it is, as this goes beyond the standards of what we normally expect. As this is the perspective I normally review PRs from, I am submitting a preliminary approval, **even if this is not 100% ready to merge**. If everything else (accuracy of instructions, screenshot) are good, I'm fine with merging this as-is. Overall, great job, and thank you.
@ -0,0 +97,4 @@
## Deployment Methods
You can deploy the runner in different environments, depending on your current setup and how much you care about security.
Owner
Copy link

Content-wise, this is a nice "bridge" / introduction, but it's kind of leaving me confused. Maybe this sentence should be rewritten. "in different environments" is confusing me a bit. Rephrasing "how much you care about security" to "security needs" may also be better.

"how much you care about security" is a bit disorienting, as it insinuates that the infrastructure may be more prone to compromise if the wrong decision is taken, without much guidance as to how to achieve an optimal setup.


Here's two examples that I would build on top of:
Depending on your setup and your security needs, there is a wide variety of options for deploying [...]
There are various methods for deploying a Runner, choosing the right one depends on your setup and your security needs.

Content-wise, this is a nice "bridge" / introduction, but it's kind of leaving me confused. Maybe this sentence should be rewritten. "in different environments" is confusing me a bit. Rephrasing "how much you care about security" to "security needs" may also be better. "how much you care about security" is a bit disorienting, as it insinuates that the infrastructure may be more prone to compromise if the wrong decision is taken, without much guidance as to how to achieve an optimal setup. --- Here's two examples that I would build on top of: `Depending on your setup and your security needs, there is a wide variety of options for deploying [...]` `There are various methods for deploying a Runner, choosing the right one depends on your setup and your security needs.`
oatbiscuits marked this conversation as resolved
@ -0,0 +115,4 @@
{% endadmonition %}
You will need to get the GID of docker in your host machine by running:
Owner
Copy link

Why? Also, it would be cool if you could consistently capitalize names of tools like "docker" -> "Docker".

Why? Also, it would be cool if you could consistently capitalize names of tools like "docker" -> "Docker".
oatbiscuits marked this conversation as resolved
@ -0,0 +253,4 @@
mountPath: /certs
```
Make sure to replace the value of `token` with your registration token.
Owner
Copy link

This could work as an admonition. (warning)

This could work as an admonition. (warning)

(totally didn't forget the fact that the new page has to be added to the main CI page and had it brought by the OP)

(totally didn't forget the fact that the new page has to be added to the main CI page and had it brought by the OP)
Author
Contributor
Copy link

Thank you for the great feedback and kind words. I made the appropriate amendments, and added two screenshots.

Thank you for the great feedback and kind words. I made the appropriate amendments, and added two screenshots.
oatbiscuits changed title from (削除) WIP: Add documentation for Forgejo CI Actions runner (削除ここまで) to Add documentation for Forgejo CI Actions runner 2023年11月19日 10:59:12 +01:00
linus left a comment
Copy link

Nice work! Left a couple of comments, but for the most part this LGTM :)

Nice work! Left a couple of comments, but for the most part this LGTM :)
@ -0,0 +5,4 @@
parent: CI
---
Recently, a GitHub Actions-compatible feature has been integrated in Forgejo,
First-time contributor
Copy link

Would probably rewrite this to drop Recently; it's not relevant here.

Would probably rewrite this to drop _Recently_; it's not relevant here.
oatbiscuits marked this conversation as resolved
@ -0,0 +41,4 @@
<picture>
<source srcset="/assets/images/ci/actions/create-new-runner.webp" type="image/webp">
<img src="/assets/images/ci/actions/create-new-runner.png" alt="Create new Runner">
First-time contributor
Copy link

Nit: different indentation

Nit: different indentation
oatbiscuits marked this conversation as resolved
@ -0,0 +51,4 @@
- Download the binary to run on your machine:
```bash
First-time contributor
Copy link

nit: the correct language tag for this is console: https://stackoverflow.com/a/49004070

nit: the correct language tag for this is `console`: https://stackoverflow.com/a/49004070
Author
Contributor
Copy link

@linusg That is not true in the syntax highlighter or markdown formatter the documentation is using. Applying console instead of bash breaks the syntax highlighting.

bash is valid even in the given context (GitHub Markdown format), you'll find the citations if you scroll in the replies or other answers in the page you linked.

@linusg That is not true in the syntax highlighter or markdown formatter the documentation is using. Applying `console` instead of `bash` breaks the syntax highlighting. `bash` is valid even in the given context (GitHub Markdown format), you'll find the citations if you scroll in the replies or other answers in the page you linked.
First-time contributor
Copy link

It is valid, sure, but semantically not correct :)

I don't know what highlighter the docs use, but here on Forgejo console yields better results (first is console, second is bash):

$ command
output
$ another command
$ command
output
$ another command
It is valid, sure, but semantically not correct :) I don't know what highlighter the docs use, but here on Forgejo `console` yields better results (first is `console`, second is `bash`): ```console $ command output $ another command ``` ```bash $ command output $ another command ```
n0toose marked this conversation as resolved
@ -0,0 +60,4 @@
- Or download the container image and run it using Docker:
```bash
First-time contributor
Copy link

likewise

likewise
n0toose marked this conversation as resolved
@ -0,0 +75,4 @@
Binary:
```bash
First-time contributor
Copy link

likewise

likewise
n0toose marked this conversation as resolved
@ -0,0 +77,4 @@
```bash
$ ./forgejo-runner register --no-interactive --token {TOKEN} --name {NAME} --instance https://codeberg.org
$ INFO Registering runner, arch=amd64, os=linux, version=3.2.0.
First-time contributor
Copy link

I doubt the $ here are part of the output? looks like a prompt, I'd definitely avoid that.

I doubt the `$` here are part of the output? looks like a prompt, I'd definitely avoid that.
oatbiscuits marked this conversation as resolved
@ -0,0 +85,4 @@
Docker:
```bash
First-time contributor
Copy link

likewise

likewise
n0toose marked this conversation as resolved
@ -0,0 +113,4 @@
Podman can work by executing:
```bash
First-time contributor
Copy link

likewise (I'll let you find the rest yourself :))

likewise (I'll let you find the rest yourself :))
n0toose marked this conversation as resolved
@ -0,0 +152,4 @@
image: docker:dind
privileged: true
volumes:
- certs:/certs
First-time contributor
Copy link

nit: indentation is four spaces here

nit: indentation is four spaces here
oatbiscuits marked this conversation as resolved
@ -0,0 +293,4 @@
<picture>
<source srcset="/assets/images/ci/actions/demo-workflow.webp" type="image/webp">
<img src="/assets/images/ci/actions/demo-workflow.png" alt="Demo workflow running successfully">
First-time contributor
Copy link

nit: indentation

nit: indentation
oatbiscuits marked this conversation as resolved

if you show me up again like that it's a ban!!!!

jk, i should still have paid more attention to the indentation stuff as i deliberately glossed over the code blocks / html tags and just read the text; thank you very much for the help @linusg :D

if you show me up again like that it's a ban!!!! jk, i should still have paid more attention to the indentation stuff as i deliberately glossed over the code blocks / html tags and just read the text; thank you very much for the help @linusg :D
Author
Contributor
Copy link

@linusg Thank you for the feedback!

@linusg Thank you for the feedback!
n0toose left a comment
Copy link

made some final minor improvements, will merge as soon as i get a final confirmation

made some final minor improvements, will merge as soon as i get a final confirmation
n0toose left a comment
Copy link

PR scope expanded by request of the author, not to be merged yet

PR scope expanded by request of the author, not to be merged yet
Author
Contributor
Copy link

@n0toose should be good to merge!

@n0toose should be good to merge!

Thanks for your tenacity and for your contributions, again, awesome work! ❤️

Thanks for your tenacity and for your contributions, again, awesome work! ❤️
Sign in to join this conversation.
No reviewers
Labels
Clear labels
Codeberg Pages
Issues affecting Codeberg Pages
Documentation Usability
Issues related to using and reading docs.codeberg.org
Forgejo
Good First Issue! 👋
Kind: Bug
Kind: Documentation
Kind: Enhancement
Kind: Feature
Kind: Question
Kind: Security
Licensing
Part: Generator
This is related to the generation of the documentation, not to the content itself
Priority: High
The priority is high
Priority: Low
The priority is low
Priority: Medium
The priority is medium
Reviewed: Confirmed
Something has been confirmed
Reviewed: Duplicate
Something exists already
Reviewed: Invalid
Something was marked as invalid
Reviewed: Wontfix
Something won't be fixed
Status: Blocked
Status: Help wanted
Contributions are welcome!
Status: In progress
Work is in progress
Status: Needs feedback
Feedback is needed
Status: Ready for Review
Work is completed
Status: Review
Review is in progress / Reviewers wanted
Status: Stale
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Codeberg/Documentation!369
Reference in a new issue
Codeberg/Documentation
No description provided.
Delete branch ":ci/actions"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?