-
-
Notifications
You must be signed in to change notification settings - Fork 512
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?
Conversation
* Update CONTRIBUTING.md * Update CONTRIBUTING.md * Update CONTRIBUTING.md * Update CONTRIBUTING.md * Update CONTRIBUTING.md
Sounds interesting. Can you maybe throw together a Markdown explaining how to do that and the script and make a Pr against the contributor_guide branch for testing? @quantumryuu |
Linking my comment here for visibility: #665 (comment) |
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.
Hope you're okay with some comments :)
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.
|
||
### 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 comment
The 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 comment
The 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 comment
The 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)
|
||
--- | ||
|
||
## 11. **Best Practices Checklist** |
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.
Same as the other file
As a general comment I think that it would be a good idea to enforce some of the rules using GitHub Actions workflows. I have seen that https://github.com/community-scripts/ProxmoxVE/blob/main/.github/check-script.yml exists, but it is not in the right folder so it will not be used. It should be possible to programmatically check for rules that are set in the contributing guide. That script is already a good start. I would suggest splitting up the checks in steps and using an action to comment the results in the PR so that it is easy for people to understand why their pipelines are not passing. So taken from the list in the markdown files and that script:
By running all these checks a PR reviewer can be certain that the PR conforms to the basic guidelines. Without passing pipelines you don't need to review (although you may want to put that in the PR template and contributing guide so that people do no expect reviews for failing PRs) |
Co-authored-by: Sébastiaan <[email protected]>
### 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. |
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.
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 comment
The 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.
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I think this sentence can be dropped.
It is in the similar context as the "why coding standards are useful".
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.
Not sure if adding a link to a PR to this doc helps. Rather than a checkbox in the PR template like "Followed and applied Coding standards" may be enough.
Why removing this paragraph?
It doesn't really add new information on what Coding standards itself. Following the rule of keeping it crisp and less is more.
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.
Yeah, like above, on second tougth it seems a bit overengineered.
## 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) |
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 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:
## IDE setup
Depending on your IDE, here are some recommendations for IDE configurations supporting you in development.
### VSCode
<Recommended extensions here>
### vim
....
This way, it feels optional and other people can add other IDEs.
Furthermore, I suggest that this is an own document like ide.md
.
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 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 comment
The 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:
- It is nice to choose a single IDE in your contributor guide, that way you don't have to answer questions from people when they try something else (thus making it non-optional in the guide)
- Any seasoned (and thus stubborn) engineer will still try to use their own IDE anyways (looking at myself in the mirror), but they probably also know what they're doing
- Any code style guideline should be in place using CI pipelines and with clearly defined tools to be able to enforce that if people use other IDEs they can still contribute while conforming to the same standards
@se-bastiaan @andygrunwald Thank you to the both of you for investing time to read and improve these documents. I highly appreciate it! Some changes i have allready commited, i change the other things around in a few days when the holdays are over and íve got more sparetime. You are welcome to contribute and open a Pull Reqest against the contributer_guide branch if you have any more improvements/changes. |
### 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to start a new script from a template.
Why not going all in and providing the CLI commands for this?
If you plan to implement a new application script, use our templates to get a kickstart:
$ NEW_APP_NAME="my-script"
$ cp ct/AppName.sh ct/${NEW_APP_NAME}.sh
$ cp ...
$ sed ... (some git remote -v magic to get the fork path)
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.
Maybe it is better to move this chapter to a
Guide: Create your own App Script
which starts with
- Fork this Repository
- Kickstart your app with your template
- ...
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.
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 will start testing, experementing with this when the hollidays are over.
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 comment
The 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. |
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.
Maybe it make sense to extract this to a structure.md
.
This document can explain
- the folder structure or this repository
- the structure of some key files (e.g. function libraries)
- the structure of an application (which files build an application) <- that's the two paragraphs here
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‘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) |
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.
Here i am a bit lost. Where do I start now to build my own script? Here or with the template of ct/AppName.sh
and install/AppName-install.sh
?
## 🤝 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`. |
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.
Overall this sounds like the typicall GitHub Flow style, which is reasonable.
Only Step 4 is special to this repository.
What do you think about this suggestion:
- Making a small guide "Setting up your development environment" -> This is essentially Step 4
- For the rest, refer to guides like https://github.blog/developer-skills/github/beginners-guide-to-github-creating-a-pull-request/
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 comment
The 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.
## 📚 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the purpose of this chapter?
What goes in here? What does not go in here?
I think all relevant pages are linked above in the respective context. I am suggesting to drop this chapter.
@@ -0,0 +1,231 @@ | |||
# **AppName<span></span>.sh Scripts** | |||
`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 comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity. Why snipeit? Is this a very easy example?
I am asking, because a more popular application may create more engagement. I, at least, don't know "snipeit".
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 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.
Example: | ||
```bash | ||
# Copyright (c) 2021-2024 community-scripts ORG | ||
# Author: [YourUserName] |
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.
Why is the author needed?
Afaik by contributing upstream + the git history, this is an outdated information very soon.
I also ask myself in which situation this will help?
|
||
### 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 comment
The 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 comment
The 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 -
.
### 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. | ||
- `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 comment
The reason will be displayed to describe this comment to others. Learn more.
A good and a bad example might be good here.
What does "overdo" mean? How many tags are too much? Is there a proxmox length limit?
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.
There is no hard limit on the proxmox side. Its just there as some sorte of information.
## 5. **Contribution checklist** | ||
|
||
- [ ] Shebang is correctly set (`#!/usr/bin/env bash`). | ||
- [ ] Correct link to *build.func* | ||
- [ ] Metadata (author, license) is included at the top. |
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 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 comment
The 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.
@@ -0,0 +1,231 @@ | |||
# **AppName<span></span>.sh Scripts** |
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.
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.
[...]
APP="SnipeIT"
var_tags="assat-management;foss"
var_cpu="2"
var_ram="2048"
var_disk="4"
# The operating system. Possible values are alpine, debian, ubuntu
var_os="debian"
# ...
var_version="12"
var_unprivileged="1"
[...]
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.
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.
@@ -0,0 +1,93 @@ | |||
#!/usr/bin/env bash |
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.
This is nearly what I suggested with "the documented version" (see above).
I think this file should not be here as a single file, but included in the markdown file.
@@ -0,0 +1,265 @@ | |||
|
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.
Same suggestion as above with the "inline comment version"
About the Metadata (author, license) is included at the top. I created a workflow and tested it, can do a PR so the team can test it too!
@michelroegl-brunner if you're ok with the idea, shall I open a PR? @se-bastiaan about the Variables follow naming conventions. (if possible somehow). Care to give some examples? All I can think of for a check could be the mysql setup probably (DB_NAME, DB_USER, DB_PASS). |
I really don't know what the examples would be, I've mostly read this contribution guide and according to the docs there would be a difference, I just don't know what they would be. However, to me this is the least important workflow that could exist. I'm looking into creating a shfmt workflow however. There seem to be quite some actions available but none of them are very well maintained. |
Relevant PR for the metadata check is up with all the required checks (text & lines) |
Example for development: | ||
```bash | ||
source <(curl -s https://raw.githubusercontent.com/[USER]/[REPO]/refs/heads/[BRANCH]/misc/build.func) | ||
``` |
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Check this comment.
The scripts I've created, replace every URL!
On my end it works, and I can start creating helper scripts really fast without having to remember to change the links etc.
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.
Nice! We can resolve this comment then
✍️ Description
remove old contributing.md
add new contributing.md
First Draft, added some functionality and descriptions later
Every @community-scripts/contributor are welcome to improve this PR
🛠️ Type of Change
Please check the relevant options:
✅ Prerequisites
The following steps must be completed for the pull request to be considered:
📋 Additional Information (optional)
Provide any extra context or screenshots about the feature or fix here.