Update CONTRIBUTING.md and split by language #2386

mheon · 2025-03-24T17:56:32Z

This now includes separate files for Go and Rust, plus a central file with generic information that applies to all contributions. This will let us get Netavark and Aardvark onboarded onto the standard containers/common Contributing docs.

Honny1

Thanks, Changes LGTM. But I have some small nits that are not blocking.

Honny1 · 2025-03-24T18:58:57Z

CONTRIBUTING.md

 ```
 Fixes: #00000
-Fixes: https://github.com/containers/common/issues/00000
+Fixes: https://github.com/containers/netavark/issues/00000


I think it's an unnecessary change. Maybe it could be like this:

Suggested change

Fixes: https://github.com/containers/netavark/issues/00000

Fixes: https://github.com/containers/<repository-name>/issues/00000

Agree, although using the full link for github issues seems wrong as it really should only be the number IMO.
If you like to close an issue on another repo the actual syntax is org/repo#xxxx. I don't think the auto closing works with the full link.

Honny1 · 2025-03-24T19:02:08Z

CONTRIBUTING.md

+In order to keep your branch up to date you should rebase.

-In order to do so add the upstream repo as remote in git, i.e. for containers/common use:
+In order to do so add the upstream repo as remote in git, i.e. for containers/netavark use:


I think it's an unnecessary change. Maybe it could be like this:

Suggested change

In order to do so add the upstream repo as remote in git, i.e. for containers/netavark use:

In order to do so add the upstream repo as remote in git, i.e. for containers/<repository-name> use:

Honny1 · 2025-03-24T19:02:27Z

CONTRIBUTING.md

+In order to do so add the upstream repo as remote in git, i.e. for containers/netavark use:
 ```
-$ git remote add upstream git@github.com:containers/common.git
+$ git remote add upstream git@github.com:containers/netavark.git


I think it's an unnecessary change. Maybe it could be like this:

Suggested change

$ git remote add upstream git@github.com:containers/netavark.git

$ git remote add upstream git@github.com:containers/<repository-name>.git

Honny1 · 2025-03-24T19:03:15Z

CONTRIBUTING.md

-For example assume you did a Podman update and now something that used to work fine is no longer working,
-this is called a regression. If the change was not intentional it may be hard to find out what caused it.
-git bisect can help with that.
+For example, assume you updated Netavark and now something that used to work fine is no longer working.


Suggested change

For example, assume you updated Netavark and now something that used to work fine is no longer working.

For example, assume you updated Podman and now something that used to work fine is no longer working.

This comment is still open.

Luap99 · 2025-03-25T10:54:10Z

CONTRIBUTING.md

+Please make sure to read the contributing docs in each repository as they may do things differently.

-This documented is primarily aimed at the following repositories:
+This is a generic document aimed at all repositories.


This seems wrong, the reason I listed the repos is because we have plenty repos not maintained by us that have their own process. As such I strongly think it is important to list only our repos.

Luap99 · 2025-03-25T10:55:11Z

CONTRIBUTING.md

+This is a generic document aimed at all repositories.
+Rules specific to the Rust language can be found [here](./CONTRIBUTING_RUST.md).
+Rules specific to the Go language can be found [here](./CONTRIBUTING_GO.md).
+We recommend you read the Rust guidelines after finishing with this file.


Not sure why this is said here, if you only work on go there is no need to rust the rust file.

Luap99 · 2025-03-25T10:55:54Z

CONTRIBUTING.md

- [psgo](https://github.com/containers/psgo)
-
-However most of the things here listed are very generic when contributing to public projects
+This document is primarily aimed at repositories within the Containers Github organization.


again I think this is wrong as we have projects here that do not follow our process at all.

Luap99 · 2025-03-25T10:57:53Z

CONTRIBUTING.md

-* Additional tests. Ideally, they should fail w/o your code change applied. A test can be a unit test
-  (`..._test.go` files next to the code you changed) or in a more complex suite often found in the
+* Additional tests. Ideally, they should fail without your code change applied. A test can be a unit test
+  (which should be added in the same file as the code being tested) or in a more complex suite often found in the


this is wrong! Go tests are never in the same file. You likely should not mention this here at all and then only the go/rust file defines how a unit test looks like.

Luap99 · 2025-03-25T10:59:42Z

CONTRIBUTING.md

 ```
 Fixes: #00000
-Fixes: https://github.com/containers/common/issues/00000
+Fixes: https://github.com/containers/netavark/issues/00000


Agree, although using the full link for github issues seems wrong as it really should only be the number IMO.
If you like to close an issue on another repo the actual syntax is org/repo#xxxx. I don't think the auto closing works with the full link.

mheon · 2025-03-25T13:33:44Z

While we're at it... @inknos anything you'd want added here for podman-py?

inknos · 2025-03-25T14:08:14Z

CONTRIBUTING.md

+Rules specific to the Rust language can be found [here](./CONTRIBUTING_RUST.md).
+Rules specific to the Go language can be found [here](./CONTRIBUTING_GO.md).


since podman-py is the only python project we have, I believe including a cross ref to https://github.com/containers/podman-py/blob/main/CONTRIBUTING.md is enough. I wouldn't do CONTRIBUTING_PYTHON.md so maybe here?

mheon · 2025-03-27T19:25:03Z

Comments addressed, should be ready

inknos · 2025-03-27T19:46:25Z

LGTM

Luap99

LGTM overall but the one comment from @Honny1 is still open an should be fixed

Luap99 · 2025-03-28T13:47:30Z

CONTRIBUTING.md

-For example assume you did a Podman update and now something that used to work fine is no longer working,
-this is called a regression. If the change was not intentional it may be hard to find out what caused it.
-git bisect can help with that.
+For example, assume you updated Netavark and now something that used to work fine is no longer working.


This comment is still open.

This now includes separate files for Go and Rust, plus a central file with generic information that applies to all contributions. This will let us get Netavark and Aardvark onboarded onto the standard containers/common Contributing docs. Signed-off-by: Matt Heon <mheon@redhat.com>

mheon · 2025-03-28T16:54:16Z

Comment addressed, anyone want to merge?

Luap99

/lgtm

openshift-ci · 2025-03-28T16:55:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, Luap99, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99,mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the approved label Mar 24, 2025

Honny1 approved these changes Mar 24, 2025

View reviewed changes

This was referenced Mar 25, 2025

Add CONTRIBUTING.md containers/netavark#1199

Closed

Add link to c/common contributor's guide containers/netavark#1210

Merged

Add link to c/common contributor's guide containers/aardvark-dns#580

Merged

Luap99 reviewed Mar 25, 2025

View reviewed changes

mheon force-pushed the update_contributing branch from 40cea8e to 3111c4d Compare March 25, 2025 13:32

inknos reviewed Mar 25, 2025

View reviewed changes

mheon force-pushed the update_contributing branch from 3111c4d to 30ea5c7 Compare March 27, 2025 19:24

Luap99 reviewed Mar 28, 2025

View reviewed changes

mheon force-pushed the update_contributing branch from 30ea5c7 to 85568aa Compare March 28, 2025 13:56

Luap99 approved these changes Mar 28, 2025

View reviewed changes

openshift-ci bot assigned Luap99 Mar 28, 2025

openshift-ci bot added the lgtm label Mar 28, 2025

openshift-merge-bot bot merged commit 1f09589 into containers:main Mar 28, 2025
15 checks passed

	Fixes: https://github.com/containers/netavark/issues/00000
	Fixes: https://github.com/containers/<repository-name>/issues/00000

	In order to do so add the upstream repo as remote in git, i.e. for containers/netavark use:
	In order to do so add the upstream repo as remote in git, i.e. for containers/<repository-name> use:

	$ git remote add upstream git@github.com:containers/netavark.git
	$ git remote add upstream git@github.com:containers/<repository-name>.git

	For example, assume you updated Netavark and now something that used to work fine is no longer working.
	For example, assume you updated Podman and now something that used to work fine is no longer working.

		Rules specific to the Rust language can be found [here](./CONTRIBUTING_RUST.md).
		Rules specific to the Go language can be found [here](./CONTRIBUTING_GO.md).

Update CONTRIBUTING.md and split by language #2386

Update CONTRIBUTING.md and split by language #2386

Uh oh!

Conversation

mheon commented Mar 24, 2025

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Mar 27, 2025

Uh oh!

inknos commented Mar 27, 2025

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Mar 28, 2025

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Mar 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants