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

Add the steps for safety in the release procedure #1195

Merged
merged 2 commits into from
Nov 3, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Nov 3, 2021

Summary

This is related to #1194

As you may notice, version 3.8.0 did not go well. The published package file included only package.json and a few text files.

npm notice === Tarball Contents ===
npm notice 1.1kB LICENSE
npm notice 2.7kB dist/package.json
npm notice 2.4kB package.json
npm notice 258B  CHANGELOG.md
npm notice 8.3kB README.md
npm notice === Tarball Details ===

To deal with the situation, I went with the following actions:

  • I've made sure if the situation can arise not only on RunKit but with my simple example app
  • I've marked v3.8.0 as deprecated in npm package registry
  • I ran npm pack in the local machine and found that the generated file is still invalid
  • I checked the changes in package.json and build-related files since v3.7.0 but there are no related changes
  • I saw the issue disappeared when I ran npm pack after deleting dist and node_modules directories
  • I ran npm publish with a new patch version (3.8.1)
  • I've installed @slack/bolt@3.8.1 in an example app project and confirmed that the issue no longer exists

I am not yet 100% confident about the direct reason of this situation. That being said, I'm guessing that utilizing npm link for local development might affect the build process. If this is true, that explains why deleting directories helped.

Not to repeat this error in the future, I propose to add a few steps before npm publish command. Ideally, we may want to do releases from the CI runtime (at least, somewhere without any person's local environment). But, for now, we can make the current operation safer.

Requirements (place an x in each [ ])

@seratch seratch added the release label Nov 3, 2021
@seratch seratch added this to the 3.x milestone Nov 3, 2021
@seratch seratch self-assigned this Nov 3, 2021
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #1195 (3e6aa54) into main (d4e19de) will not change coverage.
The diff coverage is n/a.

❗ Current head 3e6aa54 differs from pull request most recent head 099eae2. Consider uploading reports for the commit 099eae2 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1195   +/-   ##
=======================================
  Coverage   72.22%   72.22%           
=======================================
  Files          17       17           
  Lines        1433     1433           
  Branches      430      430           
=======================================
  Hits         1035     1035           
  Misses        322      322           
  Partials       76       76           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4e19de...099eae2. Read the comment docs.

@@ -4,10 +4,11 @@

# build products
/dist
*.tgz
Copy link
Member Author

Choose a reason for hiding this comment

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

npm pack generates tgz files

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Ooooof, good catch.

+1000000 on automating publishing via CI. Most other open source projects I work on do automated releasing when a git tag is pushed up to the GitHub repository. I am happy to assist with that!

.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.

Thank you for adding these precautionary steps to the maintainer's guides. It is new to me that npm link might screw with the package generated, but it does make a lot of sense (TIL).

+1 to automated release management and I'd be interested in learning what it takes to set it up. I don't know if there's enough work involved to go around @filmaj but happy to help out in this effort!

Co-authored-by: Fil Maj <maj.fil@gmail.com>
@seratch seratch merged commit 8f5fd59 into slackapi:main Nov 3, 2021
@seratch seratch deleted the issue-1194-improvement branch November 3, 2021 21:25
@filmaj filmaj removed this from the 3.x milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants