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

Add initial PR for Contributing & Coding Standard #920

Merged
merged 29 commits into from
Jan 10, 2025
Merged

Conversation

MickLesk
Copy link
Member

@MickLesk MickLesk commented Dec 19, 2024

✍️ 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:

  • Bug fix (non-breaking change that resolves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change unexpectedly)
  • New script (a fully functional and thoroughly tested script or set of scripts)

✅ Prerequisites

The following steps must be completed for the pull request to be considered:

  • Self-review performed (I have reviewed my code to ensure it follows established patterns and conventions.)
  • Testing performed (I have thoroughly tested my changes and verified expected functionality.)
  • Documentation updated (I have updated any relevant documentation)

📋 Additional Information (optional)

Provide any extra context or screenshots about the feature or fix here.

@MickLesk MickLesk added documentation A change to documentation investigation We are looking into it guide Guides on how to perform a specific task or configuration labels Dec 19, 2024
@MickLesk MickLesk requested a review from a team as a code owner December 19, 2024 09:53
@github-actions github-actions bot added the maintenance Code maintenance or general upkeep of the project label Dec 19, 2024
@MickLesk MickLesk marked this pull request as draft December 19, 2024 10:10
@michelroegl-brunner
Copy link
Member

@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.

Comment on lines 41 to 43
### 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.
Copy link
Contributor

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

  1. Fork this Repository
  2. Kickstart your app with your template
  3. ...

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

Copy link
Contributor

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.

Comment on lines 61 to 63
## 🚀 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)
Copy link
Contributor

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?

Comment on lines 100 to 102
## 📚 Pages

- [Function-Overview](https://github.com/community-scripts/ProxmoxVE/wiki/Function_Overview)
Copy link
Contributor

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.

@quantumryuu
Copy link
Contributor

quantumryuu commented Dec 29, 2024

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:

* Check if the Shebang is correctly set (`#!/usr/bin/env bash`) (already in the existing unused workflow)

* Correct link to _build.func_ (already in the existing unused workflow)

* Check for executable permissions (already in the existing unused workflow)

* Check for empty lines at the top (already in the existing unused workflow)

* Metadata (author, license) is included at the top.

* Variables follow naming conventions. (if possible somehow)

* Check if the update function exists.

* Run ShellCheck (already done)

* Run shfmt (this is what the vs code plugin uses under the hood): https://github.com/mvdan/sh#shfmt

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)

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!
It checks if these lines exist though, could probably incorporate that these lines exist on the top.

@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).

@se-bastiaan
Copy link
Contributor

@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.

@quantumryuu
Copy link
Contributor

Relevant PR for the metadata check is up with all the required checks (text & lines)

@MickLesk
Copy link
Member Author

MickLesk commented Jan 3, 2025

Reminder for me:

  • add, that the CT and Install names must be declared in lowcase. F.e. google.sh and google-install.sh

@quantumryuu
Copy link
Contributor

Reminder for me:

* add, that the CT and Install names must be declared in lowcase. F.e. google.sh and google-install.sh

@MickLesk should I create a workflow check for the lowercase files?

@MickLesk
Copy link
Member Author

MickLesk commented Jan 3, 2025

Reminder for me:

* add, that the CT and Install names must be declared in lowcase. F.e. google.sh and google-install.sh

@MickLesk should I create a workflow check for the lowercase files?

Yes you can do

@michelroegl-brunner michelroegl-brunner marked this pull request as ready for review January 10, 2025 09:31
@MickLesk
Copy link
Member Author

We merge the actual state of PR, For now, we have something on which we can build further PRs

@MickLesk MickLesk merged commit 160c546 into main Jan 10, 2025
2 checks passed
@MickLesk MickLesk deleted the contributor_guide branch January 10, 2025 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation A change to documentation guide Guides on how to perform a specific task or configuration investigation We are looking into it maintenance Code maintenance or general upkeep of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants