Docs: improve the "Contributing" section documentation

[saragerion]

Description of the improvement

Summary of the proposal

Right now the contributing docs are very bare.
To make it inclusive and accessible for contributors from different seniority, background and experience, this area should be be more detailed to make sure that ambiguity is reduced and most importantly, in case of PR's, expectations are communicated early on in the PR progress.

How, where did you look for information

In the current Contributing doc.

Missing or unclear documentation

Contributions may be related to:

• Design (UX, accessibility)
• Research to scope requirements or understand priorities
• Improve the documentation
• Add or improve examples
• Writing tutorials
• Implementation of new features via Pull Requests's
• Bux fixes
• Share ideas & proposals
• Organising Github issues, labels, projects
• Review code and Pull Requests

Improvement

Rather than 1 single cluttered CONTRIBUITING.md file, a folder with organized files/sections might be easier to read and browse.

Related existing documentation

Contributing file

Related issues, RFCs

#426
#422

[dreamorosi]

Agree that it needs improvement. The AWS Solutions Constructs repo, in my opinion, has a good example of this file.

Also interesting is their DESIGN_GUIDELINES.md file which we could also adopt a form of.

The process they request contributors to follow requires an issue and a design document to be reviewed by a maintainer before someone starts to implement a change. For trivial changes just the issue could suffice but for bigger changes I think it's a good idea. On one hand the maintainer review could become a bottleneck but in the spirit of a good experience as a contributor it'd be far worse to invest time and effort in an implementation that'd be rejected when it becomes a PR.

[bahrmichael]

I like @dreamorosi's suggestion of the AWS Solutions Constructs example, and think we could get a good first document by using and adapting the example for our repository. I'll be happy to start a draft.

I don't have much experience around design guidance, and am quite new to this project. So for now I'll be passive regarding design guidelines.

[dreamorosi]

I think a good starting point for the design principles could be the tenets described in the Python's version of the project. Other than that I think good principles could be feature parity and consistency with other runtimes.

[saragerion]

@bahrmichael @dreamorosi thanks for your input, I completely agree.

I would also add: in our contributing guidelines we should also clarify how to find Issues, PR's (through the Github projects). Additionally we could define what "ownership" (= assignees) of an issue is and what that means in practice.

Here's my proposal:

1. When someone creates a new Issue, there is no owner assigned because the issue needs to be triaged still (we might or might not implement it for example). To get feedback of the maintainers, we can tag them in the issue's comments to get their input (and/or ping them on slack of course). So the issue has no owner (assignee) yet.
2. Once the issue is triaged and ready to be picked up, 1 or more owners can be assigned in the Assignees section. An owner of an issue has the responsibility of carry it to completion and bring it to the done column.

I myself am guilty of assigning issues to get people's attention so not following what suggested above, by the way. But I realised that this was causing me confusion.

I think documenting this etiquette will bring more clarity about who is working on what in the future. But first we need of course to agree whether this is the best way of working/approach or there are better ways.
What do you think?
cc: @alan-churley

[dreamorosi]

100% agree with this, and it's great to document it so that we all know what's the proper etiquette. To double down on this I think we could codify also how PRs should work. For those I think the current behaviour is that the assignee is the person who opened the PR and is responsible of resolve any comment/request from the reviewers. Reviewers are those who are expected to review the PR and provide feedback/approve/reject the changes.

What do you think?

[bahrmichael]

We should also include build steps they can run themselves to ensure proper linting (and maybe add a step in the PR build to verify if it's alright).

Agree on enforcing it.
Using camel casing is something we could also document here #115 because I can see how someone new to the project could do a similar thing.

#102 (comment)

I think we're reaching a point where we need to recollect all the ideas, and trim them down into individual tasks.

[saragerion]

Another item we could document: testing style.
#102 (comment)

[alan-churley]

I think the approach here seems sensible, and yes more documentation on how things should work the better!

[saragerion]

As suggested by @alan-churley this issue can include the setup of pre-commit hooks that would validate/flag errors

[saragerion]

Another important point to document (and possibly also setup in our project configuration) is the versions of:

• NPM
• Node.js
  that you need to have installed locally to have the project properly working.
  I am in favour of documentation + local setup scripts/hooks to avoid errors related to wrong versions being used.

[bahrmichael]

There's been a lot of great feedback, so I'll try to summarize it into some areas that we can work through bit by bit.

• Project Configuration: Docs: improve the "Contributing" section documentation #115 (comment)
• Design Guidelines
  • https://github.com/awslabs/aws-solutions-constructs/blob/main/DESIGN_GUIDELINES.md
  • Tenets from https://awslabs.github.io/aws-lambda-powertools-python/latest/
  • "Other than that I think good principles could be feature parity and consistency with other runtimes."
• "clarify how to find Issues, PR's (through the Github projects)." Docs: improve the "Contributing" section documentation #115 (comment)
• "define what "ownership" (= assignees) of an issue is and what that means in practice" ttps://github.com/Docs: improve the "Contributing" section documentation #115#issuecomment-884737311
• How to do PRs: Docs: improve the "Contributing" section documentation #115 (comment)
• Code Style
  • Linting: Docs: improve the "Contributing" section documentation #115 (comment)
  • Testing: Docs: improve the "Contributing" section documentation #115 (comment)
  • Pre-commit hooks: Docs: improve the "Contributing" section documentation #115 (comment)

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[saragerion]

Another aspect that we should add in our contributing section: avoiding creating PR's that include code changes that are not related to the specific issue they are trying to address.
Housekeeping shows good intentions, but it does not help us maintainers keeping track of the changes that are released and it's better to create a separated issue/PR for changes unrelated to the original scope.

Also we should clarify what's the best way to define a conversation resolved in a PR review :)

[ijemmy]

Should we put contributing section in the doc?
It's easier to locate and search. But the drawback is that the content has to be in the docs folder.

[michaelbrewer]

normally it is in the root dir

[flochaz]

Should we put contributing section in the doc? It's easier to locate and search. But the drawback is that the content has to be in the docs folder.

I personally would prefer to be just a link to CONTRIBUTING.md from doc. Dev will first look at this file at root folder of repo.

[michaelbrewer]

Adding GitPod or CodeSpaces support is really handy .

eg: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md

[michaelbrewer]

I did one for the python project too, makes it super easy to contribute

CodeSpaces config:

• https://github.com/aws/aws-cdk/blob/master/.devcontainer.json

GitPod config:

• https://github.com/aws/aws-cdk/blob/master/.gitpod.yml

[dreamorosi]

Did a quick test with devcontainer in VSCode, leaving this here for future reference:

.devcontainer/Dockerfile

ARG VARIANT="16"
FROM mcr.microsoft.com/vscode/devcontainers/javascript-node:0-${VARIANT}

RUN mkdir ~/.npm-global && \
  npm config set prefix '~/.npm-global' && \
  echo "export PATH=~/.npm-global/bin:$PATH" >> ~/.profile

ARG EXTRA_NODE_VERSION=16.5.0
RUN su node -c "source /usr/local/share/nvm/nvm.sh && nvm install ${EXTRA_NODE_VERSION}"

.devcontainer/devcontainer.json

{
  "name": "Node.js",
  "build": {
    "dockerfile": "Dockerfile"
  },
  "extensions": [
    "dbaeumer.vscode-eslint",
    "esbenp.prettier-vscode",
    "visualstudioexptteam.vscodeintellicode",
    "amazonwebservices.aws-toolkit-vscode",
    "github.copilot",
    "firsttris.vscode-jest-runner"
  ],
  "postCreateCommand": "npm install && npm run lerna-bootstrap && npm run lerna-test"
}

[michaelbrewer]

I would not mind adding a PR for gitpod / CodeSpaces support. To help onboard contributors.

[saragerion]

Just FYI, I created a separate ticket for the scope of "local development" (new contributors onboarding)

#426

[michaelbrewer]

There is still typos, python references and aws cdk project references.

[michaelbrewer]

Example of referencing tests/e2e which does not exist in the repo (maybe this references within the packages/logger/tests/unit ?)

Python references, for which there still needs to be a Typescript version of this.

[ijemmy]

Most of the items are addressed (Python references are also removed). With @AWSDB PR, we're confident for Production Release with this doc.

Any other future improvement on CONTRIBUTING.md will be created as a seperated issue.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.