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

Update Maintainer's Guide #1265

Merged
merged 2 commits into from
Jun 11, 2021
Merged

Conversation

misscoded
Copy link
Contributor

Summary

The last run through the release process brought to light some deficiencies in our Maintainer's Guide. This PR aims to shore up those deficiencies and remove references to lerna.

Removing lerna all together (package.json) is out of scope for this PR, but should be revisited.

Requirements (place an x in each [ ])

@misscoded misscoded added the docs M-T: Documentation work only label Jun 10, 2021
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Great work 👍

.github/maintainers_guide.md Outdated Show resolved Hide resolved
Copy link
Member

@srajiang srajiang left a comment

Choose a reason for hiding this comment

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

@misscoded My comments are mostly questions for clarification. The changes you've made to the doc are making sense to me (with the current context I have!)

.github/maintainers_guide.md Outdated Show resolved Hide resolved
this project. If you use this package within your own software but don't plan on modifying it, this guide is
**not** for you.
## 🛠 Tools
Maintaining this project requires installing [Node.js](https://nodejs.org). All of the remaining tools are downloaded as `devDependencies`, which means you'll have them available once you run `npm install` in a working copy of this repository.
Copy link
Member

Choose a reason for hiding this comment

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

Question: When I installed the modules, I ran into some deprecation warnings for some of the listed dependencies. Would it make sense to track an issue to update these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question! The vulnerabilities look to be all associated with lerna. Since we're moving towards removing lerna all together, I don't believe it's worth fixing it beforehand, but if any if any of the other maintainers disagree on that point, we should definitely create an issue to address it.


This project has tests for individual packages as `*.spec.js` files and inside of each's respective `test` directory. It also has
integration tests in the `integration-tests` directory under the root `support` directory. You can run the entire test
suite using the npm script `npm test` at the top level. This script will use Lerna to invoke tests in each package and
Copy link
Member

Choose a reason for hiding this comment

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

Question: I think I'm missing a bit of context here on the motivation for removing all README references to Lerna.

When I run npm test and npm lint in the packages it seems like Lerna is still handling this, so it would make sense to me to keep some mention that Lerna is doing this locally, until the dependency is fully removed in a later PR. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a look because I wanted to make sure I didn't overlook anything, but lerna is only referenced at the root level package.json: none of the individual packages rely on it. I would have removed it entirely, but Steve hypothesized that we might cause CI errors, so I've omitted that from this PR.

Are you running these commands from the root level?

.github/maintainers_guide.md Outdated Show resolved Hide resolved
Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

Big improvement! 🌮


## Everything else
### 📬 Triage
Triaging is the process of investigating new issues, assigning them an appropriate label, and responding to the submitting developer. An issue should have **one** of the following labels applied: `bug`, `enhancement`, `question`, `needs feedback`, `docs`, `tests`, or `discussion`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Triaging is the process of investigating new issues, assigning them an appropriate label, and responding to the submitting developer. An issue should have **one** of the following labels applied: `bug`, `enhancement`, `question`, `needs feedback`, `docs`, `tests`, or `discussion`.
Triaging is the process of investigating new issues, assigning them an appropriate label, and responding to the submitting developer. An issue should have **one** of the following labels applied: `bug`, `enhancement`, `question`, `needs info`, `docs`, `tests`, or `discussion`.

@misscoded misscoded merged commit 5c41ab8 into slackapi:main Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs M-T: Documentation work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants