[WIP] added contributing.md file to assist new contributors #946

kernelpanic77 · 2022-10-06T11:12:29Z

Signed-off-by: kernelpanic77 shanware.ishan@gmail.com

What does this PR do?

This PR completes the addition of the contributing guidelines to the DWO repository.

What issues does this PR fix or reference?

#935

Is it tested? How?

PR Checklist

E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- v8-devworkspace-operator-e2e: DevWorkspace e2e test
- v8-che-happy-path: Happy path for verification integration with Che

Signed-off-by: kernelpanic77 <shanware.ishan@gmail.com>

openshift-ci · 2022-10-06T11:14:33Z

Hi @kernelpanic77. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

amisevsk

Thanks @kernelpanic77. I know this is WIP but I had a few initial suggestions.

docs/CONTRIBUTING.md

amisevsk · 2022-10-07T01:18:10Z

docs/CONTRIBUTING.md

+    (#testing-your-changes)
+  - [Signing-off on Commits](#signing-off-on-commits)


Accidentally duplicated lines?

Yes, updated.

amisevsk · 2022-10-07T01:19:44Z

docs/CONTRIBUTING.md

+git remote -v
+```
+
+**6.** Always take a pull from the upstream repository to your master branch to keep it at par with the main project (updated repository).


We should avoid focusing on how to set up git repositories for the contributing guide -- assume the user has cloned a suitable repo to their local machine and wants to begin writing code for it. To this end, focus on

What's required

What the flow is for starting/debugging/testing

Yes, I have added a "testing your changes" section for describing the general workflow of debugging and testing.

amisevsk · 2022-10-07T01:20:52Z

docs/CONTRIBUTING.md

+git checkout -b <your_branch_name>
+```
+
+**8.** Start the minikube cluster.


This should probably be scoped to a "developing on minikube" section; we don't require developers use minikube. In addition, how to start/configure minikube is out of scope for this article except where required (e.g. if we needed a specific amount of memory)

Hi @amisevsk! I have added that the steps are specifically for developing the devworkspace-operator on a minikube cluster. Should I add anything else ?

amisevsk · 2022-10-07T01:21:43Z

docs/CONTRIBUTING.md

+**12.** Install the dependencies for running the devworkspace-operator in your cluster.
+
+```
+make install
+```
+
+**13.** Scale down the replicas of pods to 0.
+
+```
+kubectl patch deployment/devworkspace-controller-manager --patch "{\"spec\":{\"replicas\":0}}" -n $NAMESPACE
+```
+
+**14.** Run the devworkspace-operator.
+
+```
+make run
+```


Structurally, this should appear in a section like "running the controller locally"; this is not the only way to contribute so we shouldn't present it as such.

AObuchow

@kernelpanic77 looking great so far 😎 In addition to @amisevsk's review, left some minor comments and potential suggestions.

After these comments are addressed, it might be worth adding a section on developing the webhook. IIRC, this process involves:

Make a change to the webhook
Ensure the DWO_IMG environment variable points to your container image repository, eg. export DWO_IMG=quay.io/aobuchow/dwo-webhook:next
Run make docker restart (assuming DWO is already deployed to the cluster, otherwise make docker install)
Wait for the webhook deployment to update with your image that contains your latest changes.

There may be a better way to develop the webhook, but this is generally the flow that I've performed.

AObuchow · 2022-10-07T14:37:28Z

docs/CONTRIBUTING.md

+
+Follow the following instructions to start contributing.
+
+**1.** Fork [this](https://github.com/devfile/devworkspace-operator) repository.


Very minor, but I would change this to Fork the [DevWorkspace Operator](https://github.com/devfile/devworkspace-operator) repository

@kernelpanic77 looking great so far sunglasses In addition to @amisevsk's review, left some minor comments and potential suggestions.

After these comments are addressed, it might be worth adding a section on developing the webhook. IIRC, this process involves:

Thanks @AObuchow. This makes sense, I'll add a specific sub section for testing and developing webhooks in the "testing your changes" section.

Make a change to the webhook

Ensure the DWO_IMG environment variable points to your container image repository, eg. export DWO_IMG=quay.io/aobuchow/dwo-webhook:next

Run make docker restart (assuming DWO is already deployed to the cluster, otherwise make docker install)

Wait for the webhook deployment to update with your image that contains your latest changes.

There may be a better way to develop the webhook, but this is generally the flow that I've performed.

I have not contributed to developing webhooks yet 😅 , so this looks right to me. @amisevsk @ibuziuk would you like to add any steps to the process specified by @AObuchow.

AObuchow · 2022-10-07T14:55:07Z

docs/CONTRIBUTING.md

+export NAMESPACE="devworkspace-controller"
+```
+
+**11.** Install the kubernetes certificate management controller to generate and manage TLS certificates for your cluster.


It's worth noting that the cert manager is required only for Kubernetes, so if a "developing on minikube" section of this documented is created, this should probably be specific to it.

Cert-manager is required for all deployments on Kubernetes, not just minikube.

Yes, adding this comment in the steps. Thanks @AObuchow @amisevsk.

AObuchow · 2022-10-07T14:58:23Z

docs/CONTRIBUTING.md

+**11.** Install the kubernetes certificate management controller to generate and manage TLS certificates for your cluster.
+
+```
+make install cert-manager.


I've always used make install_cert_manager as this is the rule defined here and also referenced in the README.

Though, make install cert-manager does seem to work? Not sure where this recipe is defined as I couldn't find it when grepping the repo.

yes, making the change. make install_cert_manager seems right to me as well.

AObuchow · 2022-10-07T15:17:40Z

docs/CONTRIBUTING.md

+make install
+```
+
+**13.** Scale down the replicas of pods to 0.


Maybe "Scale down the controller manager deployment's pods to 0."?

The reason this step is necessary is that only 1 controller manager can be run at a time (otherwise their operations will conflict/"fight" with each other), and if we are running the controller locally then we need to ensure the controller is not running on the cluster.

Noted 👍 .

AObuchow · 2022-10-07T15:25:59Z

docs/CONTRIBUTING.md

+make run
+```
+
+This will run the devworkspace-controller in your local cluster (minikube/openshift).


Technically, this will make the devworkspace-controller run locally on your system (not on your local cluster).
I would remove the mention of "(minikube/openshift)" and, as @amisevsk mentioned, make this appear in a section on running the controller locally.

Noted 👍 .

AObuchow · 2022-10-07T15:32:11Z

docs/CONTRIBUTING.md

+git commit -s -m "<commit subject>"
+```
+
+**17.** While you are working on your branch, other developers may update the `master` branch with their branch. This action means your branch is now out of date with the `master` branch and missing content. So to fetch the new changes, follow along:


As @amisevsk mentioned, I would omit instructions on setting up git/developing with git. With that being said, for DWO the master branch is called main.

Yes, I have removed the git/developing instructions for commit instructions.

AObuchow · 2022-10-07T15:39:40Z

docs/CONTRIBUTING.md

+
+- [Contributing to Devworkspace-Operator](#contributing-to-devworkspace-operator)
+- [Before You Get Started](#before-you-get-started)
+  - [Code of Conduct](#code-of-conduct)


I'm not sure if you had ideas for the "For Newcomers" section, but if you end up omitting it, it may be worth moving the code of conduct section to the end of this document (or at least, after the "How to Contribute" section).

This is just so that the contributing instructions are immediately visible (as the Code of Conduct section could be a bit lengthy).

Also, if you're looking for inspiration for a code of conduct, here's one

Actually, here's an even better Code of Conduct from the devfile API repo: https://github.com/devfile/api/blob/main/CODE_OF_CONDUCT.md. It's probably worth making another, separate file for the code of conduct and linking to it in the CONTRIBUTING.md

I am actually omitting the newcomer's section. As it just doesn't seem an immediate requirement for now.

docs/CONTRIBUTING.md

openshift-ci · 2022-11-10T06:09:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kernelpanic77
Once this PR has been reviewed and has the lgtm label, please assign amisevsk for approval by writing /assign @amisevsk in a comment. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

ibuziuk

@kernelpanic77 thank you for your interest in the project. Would you be able to address the review comments and rebase?

Co-authored-by: Angel Misevski <amisevsk@redhat.com>

kernelpanic77 · 2023-01-13T17:01:44Z

Hi @ibuziuk ! I apologize for the delay in addressing these comments. I am adding new commits for the changes requested by @AObuchow @amisevsk.

Signed-off-by: kernelpanic77 <shanware.ishan@gmail.com>

Co-authored-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: kernelpanic77 <shanware.ishan@gmail.com>

kernelpanic77 · 2023-01-24T20:21:08Z

Hi @ibuziuk @AObuchow @amisevsk! I have reviewed comments and rebased the PR. Pls let me know if anymore changes are needed.

amisevsk · 2023-01-24T20:42:25Z

@kernelpanic77 I think something went wrong with your rebase, this PR is now showing as containing 111 commits, changing 322 files.

kernelpanic77 · 2023-01-25T10:20:11Z

@amisevsk I have fixed the issue. I changed the base branch to main.

AObuchow

@kernelpanic77 Awesome work 😎 🙏 Looking forward to getting this merged.

I left some comments, most of the things I wrote are minor nitpicks :P

AObuchow · 2023-01-26T16:56:39Z

docs/CONTRIBUTING.md

@@ -0,0 +1,126 @@
+# Contributing to Devworkspace-Operator


For consistency with the README, the project should be referred to as "DevWorkspace Operator". @amisevsk correct me if I am wrong.

docs/CONTRIBUTING.md

kernelpanic77 · 2023-02-07T19:39:32Z

Hey @AObuchow ! I have addressed the changes requested by you. Please have a look. Let's get this merged 💯

added contributing.md file to assist new contributors

a53f8d8

Signed-off-by: kernelpanic77 <shanware.ishan@gmail.com>

kernelpanic77 requested review from amisevsk and ibuziuk as code owners October 6, 2022 11:12

openshift-ci bot added do-not-merge/work-in-progress needs-ok-to-test labels Oct 6, 2022

amisevsk requested a review from AObuchow October 6, 2022 19:34

amisevsk reviewed Oct 7, 2022

View reviewed changes

AObuchow requested changes Oct 7, 2022

View reviewed changes

openshift-ci bot assigned AObuchow Oct 7, 2022

Merge branch 'devfile:main' into contribute

c8dc4a9

ibuziuk reviewed Dec 9, 2022

View reviewed changes

kernelpanic77 and others added 2 commits January 13, 2023 22:29

Update docs/CONTRIBUTING.md

079f79b

Co-authored-by: Angel Misevski <amisevsk@redhat.com>

Update docs/CONTRIBUTING.md

b171c16

Co-authored-by: Angel Misevski <amisevsk@redhat.com>

kernelpanic77 and others added 5 commits January 25, 2023 00:50

added contributing.md file to assist new contributors

a370293

Signed-off-by: kernelpanic77 <shanware.ishan@gmail.com>

Update docs/CONTRIBUTING.md

1435065

Co-authored-by: Angel Misevski <amisevsk@redhat.com>

Update docs/CONTRIBUTING.md

654a59f

Co-authored-by: Angel Misevski <amisevsk@redhat.com>

updated contributing.md

0c0520a

Signed-off-by: kernelpanic77 <shanware.ishan@gmail.com>

updated contributing.md

2d9d4c7

Signed-off-by: kernelpanic77 <shanware.ishan@gmail.com>

kernelpanic77 requested review from ibuziuk, amisevsk and AObuchow and removed request for ibuziuk and amisevsk January 24, 2023 20:19

kernelpanic77 changed the base branch from main to pr-update-base-images-1640124875 January 25, 2023 10:18

kernelpanic77 requested a review from JPinkney as a code owner January 25, 2023 10:18

kernelpanic77 changed the base branch from pr-update-base-images-1640124875 to main January 25, 2023 10:18

AObuchow requested changes Jan 26, 2023

View reviewed changes

changes for contributing.md 'testing controller'

614092f

kernelpanic77 requested review from AObuchow and removed request for JPinkney February 7, 2023 19:39

		(#testing-your-changes)
		- [Signing-off on Commits](#signing-off-on-commits)


		Follow the following instructions to start contributing.

		1. Fork [this](https://github.com/devfile/devworkspace-operator) repository.

[WIP] added contributing.md file to assist new contributors #946

Are you sure you want to change the base?

[WIP] added contributing.md file to assist new contributors #946

Conversation

kernelpanic77 commented Oct 6, 2022

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

openshift-ci bot commented Oct 6, 2022

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AObuchow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kernelpanic77 Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AObuchow Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 10, 2022

ibuziuk left a comment

Choose a reason for hiding this comment

kernelpanic77 commented Jan 13, 2023 • edited Loading

kernelpanic77 commented Jan 24, 2023

amisevsk commented Jan 24, 2023

kernelpanic77 commented Jan 25, 2023 • edited Loading

AObuchow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kernelpanic77 commented Feb 7, 2023

kernelpanic77 Jan 24, 2023 •

edited

Loading

AObuchow Oct 7, 2022 •

edited

Loading

kernelpanic77 commented Jan 13, 2023 •

edited

Loading

kernelpanic77 commented Jan 25, 2023 •

edited

Loading