Skip to content

CONTRIBUTING: add some initial guidelines #6485

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

last-genius
Copy link
Contributor

Some rough guidelines on the contribution process for the project. Intended more as a starting point for a discussion.

@last-genius
Copy link
Contributor Author

MustardseedX

This comment was marked as spam.

@lindig
Copy link
Contributor

lindig commented May 27, 2025

I believe we should manage expectations by saying that commits that break existing (non-public) tests are likely to be reverted. This may sound unfair because tests are not public but I can't see how we can avoid that. The test infrastructure we have in place is almost impossible to make public, independent of any licensing issues.

@psafont
Copy link
Member

psafont commented May 27, 2025

The test infrastructure we have in place is almost impossible to make public, independent of any licensing issues.

What could be made public is the operations / features that are considered essential, and are never to be broken. With this, the existing public tests can be enhanced (unit tests or end-to-end xcp-ng ones) to a point where all the tests that are considered for reverting are public, even if it's replicated inside xenserver's testing suite.

@lindig
Copy link
Contributor

lindig commented May 27, 2025

We might have public tests not in place immediately; I am in favour to add some words about reverting commits and merge this.

@last-genius last-genius force-pushed the asv/contribution-guide branch from 5710c8d to 20070e4 Compare May 27, 2025 15:00
@last-genius
Copy link
Contributor Author

Added a quick note:

If a commit has been determined to break integration testing at a later stage,
please note that the first and safest measure will almost always be reverting
the faulty commit. Making sure critical tests are passing remains a priority
over waiting for some commit to be reworked or refactored (which can be worked
on after a revert has been done). Though we are striving to make more tests
public (with failure then being visible to all), as long as some critical tests
remain private, this will also apply to such tests (with maintainers flagging
the breakage preferrably describing at least the gist of the test).

Comment on lines +62 to +65
Commit messages (and the body of a Pull Request) should be as helpful and
descriptive as possible. If applicable, please include a description of current
behaviour, your changes, and the new behaviour. Link any appropriate
documentation, issues, or commits.
Copy link
Member

Choose a reason for hiding this comment

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

Something important to note, the commit should not explain the change, but also reason why the change is beneficial. Sometimes this is obvious, like reducing code, but other times they may only be preparing code for later changes

@psafont
Copy link
Member

psafont commented May 27, 2025

I can't think of anything in particular that the document is missing. I'd like Rob to review it, however

## Review process and merge

It can often be useful to address review suggestions with a "fixup" commit
(created manually or with the help of `git commit --amend=HASH`). This way it
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably mean git commit --fixup ;)

https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff

The following points are intended to describe what makes a contribution "good" -
easier to review, integrate, and mantain. Please follow them in your work.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo (maintain).

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