-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Add initial PR for Contributing & Coding Standard #920
base: main
Are you sure you want to change the base?
Changes from all commits
e8f0047
7a7b0fa
9ea997f
482bd08
cf7eb92
bb3077a
d2728d2
7094920
6a0f22c
483dc98
4bde4d7
fdedd55
2f095fc
3923184
a4d39c4
2ae6203
0aaea5c
784fc81
5e68a7e
d086288
2ddd79d
730abf5
b805a9e
b0d76cc
0e1f46b
4dc7a3d
acd26c4
d14a7d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
|
||
# Community Scripts Contribution Guide | ||
|
||
## **Welcome to the communty-scripts Repository!** | ||
📜 These documents outline the essential coding standards for all our scripts and JSON files. Adhering to these standards ensures that our codebase remains consistent, readable, and maintainable. By following these guidelines, we can improve collaboration, reduce errors, and enhance the overall quality of our project. | ||
|
||
### Why Coding Standards Matter | ||
|
||
Coding standards are crucial for several reasons: | ||
|
||
1. **Consistency**: Consistent code is easier to read, understand, and maintain. It helps new team members quickly get up to speed and reduces the learning curve. | ||
2. **Readability**: Clear and well-structured code is easier to debug and extend. It allows developers to quickly identify and fix issues. | ||
3. **Maintainability**: Code that follows a standard structure is easier to refactor and update. It ensures that changes can be made with minimal risk of introducing new bugs. | ||
4. **Collaboration**: When everyone follows the same standards, it becomes easier to collaborate on code. It reduces friction and misunderstandings during code reviews and merges. | ||
Comment on lines
+7
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I love the context of why this document is important, I would like to raise the question if a description on why Coding standards are usefule is really needed. I think we can assume a basic knowledge of software engineering to people who aim to read this doc. I think it is helpful to keep the barrier for contribution low. A crisp document that brings the message across helps here. My suggestion: Removing the chapter "Why Coding Standards Matter" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah seems resonable. I had a look around what other documents like this one look like and found in many places somethings like these. |
||
|
||
### Scope of These Documents | ||
|
||
These documents cover the coding standards for the following types of files in our project: | ||
|
||
- **`install/$AppName-install.sh` Scripts**: These scripts are responsible for the installation of applications. | ||
- **`ct/$AppName.sh` Scripts**: These scripts handle the creation and updating of containers. | ||
- **`json/$AppName.json`**: These files store structured data and are used for the website. | ||
|
||
Each section provides detailed guidelines on various aspects of coding, including shebang usage, comments, variable naming, function naming, indentation, error handling, command substitution, quoting, script structure, and logging. Additionally, examples are provided to illustrate the application of these standards. | ||
|
||
By following the coding standards outlined in this document, we ensure that our scripts and JSON files are of high quality, making our project more robust and easier to manage. Please refer to this guide whenever you create or update scripts and JSON files to maintain a high standard of code quality across the project. 📚🔍 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this sentence can be dropped. Additionally, I don't know why I need to "please refer to this guide" when I am opening a PR. This doc are the guidelines to follow to create a PR. If a PR is not matching these guidelines, the feedback will be in this direction. Why removing this paragraph? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, like above, on second tougth it seems a bit overengineered. |
||
|
||
Let's work together to keep our codebase clean, efficient, and maintainable! 💪🚀 | ||
|
||
|
||
## Getting Started | ||
|
||
Before contributing, please ensure that you have the following setup: | ||
|
||
1. **Visual Studio Code** (recommended for script development) | ||
2. **Recommended VS Code Extensions:** | ||
- [Shell Syntax](https://marketplace.visualstudio.com/items?itemName=bmalehorn.shell-syntax) | ||
- [ShellCheck](https://marketplace.visualstudio.com/items?itemName=timonwong.shellcheck) | ||
- [Shell Format](https://marketplace.visualstudio.com/items?itemName=foxundermoon.shell-format) | ||
Comment on lines
+31
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the recommendation overall. However, i think many engineers have their opinionated IDE. Thats why I think we should not "phrase it as mandatory" (this is how I read it). What I can think about: Having a section like:
This way, it feels optional and other people can add other IDEs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is only a recommendation, as stated on line 35. I personaly use neovim mostly and adapted it so it fits. But i think that with one example as recommendet is more then enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hahaha I had a similar comment written earlier today but decided against adding it. For a couple of reasons:
|
||
|
||
### Important Notes | ||
- Use [AppName.sh](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/ct/AppName.sh) and [AppName-install.sh](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/install/AppName-install.sh) as templates when creating new scripts. | ||
- The call to `community-scripts/ProxmoxVE` should be adjusted to reflect the correct fork URL. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without more context it is unclear what the meaning of this sentence is (yes I know it is the call to build.func) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its better explained int App.md. Maybe remove it here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes after going through the rest of the files that would make a lot of sense. It is not important here (yet)
Comment on lines
+41
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea to start a new script from a template.
This way, folks can just copy / paste the commands and get going. Additionally: Considering the CONTIRBUTING.md is the entry doc, this is the first time it is mentioned that we operate on a fork.
which starts with
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There has been a proposal by @quantumryuu for this already, have a look here: https://github.com/community-scripts/ProxmoxVE/tree/new_script_testing/.github/CONTRIBUTOR_GUIDE/dev-scripts I also would love to have something simillar to this for Appname.sh and AppName-install.sh There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About the AppName.sh/AppName-install.sh we could modify the scripts I proposed into asking things like "Author", "Source", "AppName" I guess. |
||
|
||
--- | ||
|
||
# 🚀 The Application Script (ct/AppName.sh) | ||
|
||
- You can find all coding standards, as well as the structure for this file [here](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/ct/AppName.md). | ||
- These scripts are responsible for container creation, setting the necessary variables and handling the update of the application once installed. | ||
|
||
--- | ||
|
||
# 🛠 The Installation Script (install/AppName-install.sh) | ||
|
||
- You can find all coding standards, as well as the structure for this file [here](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/install/AppName-install.md). | ||
- These scripts are responsible for the installation of the application. | ||
Comment on lines
+47
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it make sense to extract this to a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I‘m against splitting it up even more. There should be one seperate file for every script type and one general one to not overcomplicated it. |
||
|
||
--- | ||
|
||
## 🚀 Building Your Own Scripts | ||
|
||
Start with the [template script](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/install/AppName-install.sh) | ||
Comment on lines
+61
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here i am a bit lost. Where do I start now to build my own script? Here or with the template of |
||
|
||
--- | ||
|
||
## 🤝 Contribution Process | ||
|
||
### 1. Fork the repository | ||
Fork to your GitHub account | ||
|
||
### 2. Clone your fork on your local environment | ||
```bash | ||
git clone https://github.com/yourUserName/ForkName | ||
``` | ||
|
||
### 3. Create a new branch | ||
```bash | ||
git switch -c your-feature-branch | ||
``` | ||
|
||
### 4. Change paths in build.func install.func and AppName.sh | ||
To be able to develop from your own branch you need to change `https://raw.githubusercontent.com/community-scripts/ProxmoxVE/main` to `https://raw.githubusercontent.com/[USER]/[REPOSITORY]/refs/head/[BRANCH]`. This change is only for testing. Before opening a Pull Request you should change this line change all this back to point to `https://raw.githubusercontent.com/community-scripts/ProxmoxVE/main`. | ||
Comment on lines
+67
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall this sounds like the typicall GitHub Flow style, which is reasonable. What do you think about this suggestion:
I think we will never do a better job than the Github docs in the regular content. I think we should keep the focus on the repo and create visibility on what is special here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are thinking about the contribution procces for the Pull Requests. It is not really practical how it is done at the moment, as you can not test the functionality of the script besides running code from an unknown repo or coppying the code to your one repo for testing. I would leave this as is atm. |
||
|
||
### 4. Commit changes (without build.func and install.func!) | ||
```bash | ||
git commit -m "Your commit message" | ||
``` | ||
|
||
### 5. Push to your fork | ||
```bash | ||
git push origin your-feature-branch | ||
``` | ||
|
||
### 6. Create a Pull Request | ||
Open a Pull Request from your feature branch to the main repository branch. You must only include your **$AppName.sh**, **$AppName-install.sh** and **$AppName.json** files in the pull request. | ||
|
||
--- | ||
|
||
## 📚 Pages | ||
|
||
- [Function-Overview](https://github.com/community-scripts/ProxmoxVE/wiki/Function_Overview) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This page points to nowhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was put there by @MickLesk. Is there something planed (like documentation from build.func/install.func or something like that?) from your side ore should we remove it for now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This raises a general question: Is the wiki in use or should it be used? The wiki itself is also a git repo. However, the pull request flow (incl. reviews and quality gate) is not lived there. Having docs in markdown + wiki can lead to confusion, as people asks themselves "which one is now up to date?". It may be helpful to commit to the one or the other.
Comment on lines
+100
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whats the purpose of this chapter? I think all relevant pages are linked above in the respective context. I am suggesting to drop this chapter. |
||
- [CT Template: AppName.sh](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/ct/AppName.sh) | ||
- [Install Template: AppName-install.sh](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/install/AppName-install.sh) | ||
- [JSON Template: AppName.json](https://github.com/community-scripts/ProxmoxVE/blob/contributor_guide/.github/CONTRIBUTOR_GUIDE/json/AppName.json) | ||
|
||
--- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,231 @@ | ||
# **AppName<span></span>.sh Scripts** | ||
michelroegl-brunner marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This script seem to be a "I am explaining each line in this install script". If this is the purpose, I may suggest an alternative approach to this: Copying the whole script in this markdown file and using bash comments to add the relevant documentation context. The current structure is hard to follow as the reader is not having the script in front of him/her. It is broken down in chapters. While commenting https://github.com/community-scripts/ProxmoxVE/blob/main/ct/snipeit.sh in full length, the reader is having the context visible. E.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Put sometimes you only need a short reminder of how to do handle the credentials, or how to do handle config files. If there is one finished script with comments, this would always be copied and things get left in there for no reason. This should not be a step by step guide for a script, more a how we do things here type of documentation. We could not ever explane any case in one finished script. |
||
`AppName.sh` scripts found in the `/ct` directory. These scripts are responsible for the installation of the desired application. For this guide we take `/ct/snipeit.sh` as example. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just out of curiosity. Why snipeit? Is this a very easy example? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just happens to one of my one scripts, so i know it best^^. Also it is not to complicated/long and inlcudes almost all topics wich are in the guides. This can always be changed of course. |
||
|
||
|
||
## 1. **File Header** | ||
|
||
### 1.1 **Shebang** | ||
- Use `#!/usr/bin/env bash` as the shebang. | ||
|
||
```bash | ||
#!/usr/bin/env bash | ||
``` | ||
### 1.2 **Import Functions** | ||
- Import the build.func file. | ||
- When developing your own script, change the URL to your own repository. | ||
|
||
> [!CAUTION] | ||
> Before opening a Pull Request, change the URL to point to the community-scripts repo. | ||
|
||
Example for development: | ||
```bash | ||
source <(curl -s https://raw.githubusercontent.com/[USER]/[REPO]/refs/heads/[BRANCH]/misc/build.func) | ||
``` | ||
Comment on lines
+20
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to note that this is not enough. There is also a hard-link from the build.func file into the install script: https://github.com/community-scripts/ProxmoxVE/blob/main/misc/build.func So to make sure that you actually test your own code (and I just found this out the hard way) you also need to change that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! We can resolve this comment then |
||
|
||
Example for final script: | ||
```bash | ||
source <(curl -s https://raw.githubusercontent.com/community-scripts/ProxmoxVE/main/misc/build.func) | ||
``` | ||
|
||
### 1.3 **Metadata** | ||
- Add clear comments for script metadata, including author, copyright, and license information. | ||
|
||
Example: | ||
```bash | ||
# Copyright (c) 2021-2024 community-scripts ORG | ||
# Author: [YourUserName] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the author needed? |
||
# License: MIT | https://github.com/community-scripts/ProxmoxVE/raw/main/LICENSE | ||
# Source: [SOURCE_URL] | ||
``` | ||
|
||
> [!NOTE]: | ||
> - Add your username and source URL | ||
> - For existing scripts, add "| Co-Author [YourUserName]" after the current author | ||
|
||
--- | ||
|
||
## 2 **Variables and function import** | ||
> [!NOTE] | ||
> You need to have all this set in your script, otherwise it will not work! | ||
|
||
### 2.1 **Default Values** | ||
- This section sets the default values for the container. | ||
- `APP` needs to be set to the application name and must be equal to the filenames of your scripts. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this imply things like no whitespace, no special chars, etc.? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ´think only whitespaces would be a problem, we have a couple with |
||
- `var_tags`: You can set Tags for the CT wich show up in the Proxmox UI. Don´t overdo it! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A good and a bad example might be good here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no hard limit on the proxmox side. Its just there as some sorte of information. |
||
|
||
>[!NOTE] | ||
>Description for all Default Values | ||
>| Variable | Description | Notes | | ||
>|----------|-------------|-------| | ||
>| `APP` | Application name | Must match ct\AppName.sh | | ||
>| `TAGS` | Proxmox display tags without Spaces, only ; | Limit the number | | ||
michelroegl-brunner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
>| `var_cpu` | CPU cores | Number of cores | | ||
>| `var_ram` | RAM | In MB | | ||
>| `var_disk` | Disk capacity | In GB | | ||
>| `var_os` | Operating system | alpine, debian, ubuntu | | ||
>| `var_version` | OS version | e.g., 3.20, 11, 12, 20.04 | | ||
>| `var_unprivileged` | Container type | 1 = Unprivileged, 0 = Privileged | | ||
michelroegl-brunner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Example: | ||
```bash | ||
APP="SnipeIT" | ||
var_tags="asset-management;foss" | ||
var_cpu="2" | ||
var_ram="2048" | ||
var_disk="4" | ||
var_os="debian" | ||
var_version="12" | ||
var_unprivileged="1" | ||
``` | ||
|
||
## 2.2 **📋 App output & base settings** | ||
|
||
```bash | ||
# App Output & Base Settings | ||
header_info "$APP" | ||
base_settings | ||
``` | ||
|
||
- `header_info`: Generates ASCII header for APP | ||
- `base_settings`: Allows overwriting variable values | ||
|
||
## 2.3 **🛠 Core functions** | ||
|
||
```bash | ||
# Core | ||
variables | ||
color | ||
catch_errors | ||
``` | ||
|
||
- `variables`: Processes input and prepares variables | ||
- `color`: Sets icons, colors, and formatting | ||
- `catch_errors`: Enables error handling | ||
|
||
--- | ||
|
||
## 3 **Update function** | ||
|
||
### 3.1 **Function Header** | ||
- If applicable write a function that updates the application and the OS in the container. | ||
- Each update function starts with the same code: | ||
```bash | ||
function update_script() { | ||
header_info | ||
check_container_storage | ||
check_container_resources | ||
``` | ||
|
||
### 3.2 **Check APP** | ||
- Before doing anything update-wise, check if the app is installed in the container. | ||
|
||
Example: | ||
```bash | ||
if [[ ! -d /opt/snipe-it ]]; then | ||
msg_error "No ${APP} Installation Found!" | ||
exit | ||
fi | ||
``` | ||
### 3.3 **Check version** | ||
- Befoer updating, check if a new version exists. | ||
- We use the `${APPLICATION}_version.txt` file created in `/opt` during the install to compare new versions against the currently installed version. | ||
|
||
Example with a Github Release: | ||
```bash | ||
RELEASE=$(curl -fsSL https://api.github.com/repos/snipe/snipe-it/releases/latest | grep "tag_name" | awk '{print substr($2, 3, length($2)-4) }') | ||
if [[ ! -f /opt/${APP}_version.txt ]] || [[ "${RELEASE}" != "$(cat /opt/${APP}_version.txt)" ]]; then | ||
msg_info "Updating ${APP} to v${RELEASE}" | ||
#DO UPDATE | ||
else | ||
msg_ok "No update required. ${APP} is already at v${RELEASE}." | ||
fi | ||
exit | ||
} | ||
``` | ||
### 3.4 **Verbosity** | ||
- Use the appropriate flag (**-q** in the examples) for a command to suppress its output. | ||
Example: | ||
```bash | ||
wget -q | ||
unzip -q | ||
``` | ||
- If a command does not come with this functionality use `&>/dev/null` to suppress it's output. | ||
|
||
Example: | ||
```bash | ||
php artisan migrate --force &>/dev/null | ||
php artisan config:clear &>/dev/null | ||
``` | ||
|
||
### 3.5 **Backups** | ||
- Backup user data if necessary. | ||
- Move all user data back in the directory when the update is finished. | ||
>[!NOTE] | ||
>This is not meant to be a permanent backup | ||
|
||
Example backup: | ||
```bash | ||
mv /opt/snipe-it /opt/snipe-it-backup | ||
``` | ||
Example config restore: | ||
```bash | ||
cp /opt/snipe-it-backup/.env /opt/snipe-it/.env | ||
cp -r /opt/snipe-it-backup/public/uploads/ /opt/snipe-it/public/uploads/ | ||
cp -r /opt/snipe-it-backup/storage/private_uploads /opt/snipe-it/storage/private_uploads | ||
``` | ||
|
||
### 3.6 **Cleanup** | ||
- Do not forget to remove any temporary files/folders such as zip-files or temporary backups. | ||
Example: | ||
```bash | ||
rm -rf /opt/v${RELEASE}.zip | ||
rm -rf /opt/snipe-it-backup | ||
``` | ||
|
||
### 3.7 **No update function** | ||
- In case you can not provide a update function use the following code to provide user feedback. | ||
```bash | ||
function update_script() { | ||
header_info | ||
check_container_storage | ||
check_container_resources | ||
if [[ ! -d /opt/snipeit ]]; then | ||
msg_error "No ${APP} Installation Found!" | ||
exit | ||
fi | ||
msg_error "Ther is currently no automatic update function for ${APP}." | ||
exit | ||
} | ||
``` | ||
|
||
--- | ||
|
||
## 4 **End of the script** | ||
- `start`: Launches Whiptail dialogue | ||
- `build_container`: Collects and integrates user settings | ||
- `description`: Sets LXC container description | ||
- With `echo -e "${TAB}${GATEWAY}${BGN}http://${IP}${CL}"` you can point the user to the IP:PORT/folder needed to access the app. | ||
|
||
```bash | ||
start | ||
build_container | ||
description | ||
|
||
msg_ok "Completed Successfully!\n" | ||
echo -e "${CREATING}${GN}${APP} setup has been successfully initialized!${CL}" | ||
echo -e "${INFO}${YW} Access it using the following URL:${CL}" | ||
echo -e "${TAB}${GATEWAY}${BGN}http://${IP}${CL}" | ||
``` | ||
--- | ||
|
||
## 5. **Contribution checklist** | ||
|
||
- [ ] Shebang is correctly set (`#!/usr/bin/env bash`). | ||
- [ ] Correct link to *build.func* | ||
- [ ] Metadata (author, license) is included at the top. | ||
Comment on lines
+221
to
+225
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this chapter can be removed. This file is more a "rundown of an install script". This checklist may be helpful in the PR template. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better placed here. When you finished your script and are at the bottom of the guide you can check yourself. This would make the PR template to convoluted i think. |
||
- [ ] Variables follow naming conventions. | ||
- [ ] Update function exists. | ||
- [ ] Update functions checks if app is installed an for new version. | ||
- [ ] Update function up temporary files. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this |
||
- [ ] Script ends with a helpful message for the user to reach the application. | ||
|
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 would suggest putting the main CONTRIBUTING.md in the root of the repository. This folder is way too hidden.
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, i'm totaly on your side with this. The placement can be discussed and is not final i think.
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 agree with this. Github has native support when the file is placed in root, see https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors
E.g. it is shown, when a new PR is opened to new contributors.
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.
@andygrunwald as i read it, also /docs or /.github are allowed/used for this.