-
Notifications
You must be signed in to change notification settings - Fork 504
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
Remove dep on bolt and fix 2fa stuff #106
Remove dep on bolt and fix 2fa stuff #106
Conversation
🦋 Changeset is good to goLatest commit: 5d03edd We got this. Not sure what this means? Click here to learn what changesets are. |
Codecov Report
@@ Coverage Diff @@
## master #106 +/- ##
=========================================
- Coverage 82.66% 77.07% -5.6%
=========================================
Files 31 33 +2
Lines 819 881 +62
Branches 144 160 +16
=========================================
+ Hits 677 679 +2
- Misses 131 193 +62
+ Partials 11 9 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code for this looks good (aside from removing bolt dependency) - manual testing showed a weirdness. Was running publish in: https://github.com/Noviny/test-changeset
Basically, it asked me for 2fa, then immediately asked me again, then return publishing successes. Tested this twice and got the same results.
Aside, added a mental backlog task to move this repo into changesets, and the packages into the changesets scope, so anyone with scope access can use these packages to test whatever.
package.json
Outdated
"term-size": "^2.1.0", | ||
"p-limit": "^2.2.0", | ||
"spawndamnit": "^2.0.0", | ||
"is-ci": "^2.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any instance where we are using bolt in our packages - can we remove the dependency from here entirely?
${ | ||
pkg.name | ||
} has a dependency on ${depName} at ${depRange}, however the new version of ${ | ||
// TODO: look into this, accessing internalDeps[depName] seems wrong |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these checks may still make sense, but it's more 'release plan will create invalid state' - We may want to lean on bolt-check
to do this, and filter the errors we get?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump is the thing most impacted by my rewrite, so I wouldn't worry about fixing these now, I'll be doing a pass over the messaging around running bumps as part of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I wrote a comment about changing the error message to something like that above, this comment was purely about accessing internalDeps[depName]
because internalDeps
is an array and depName
is a string so the result will always be undefined.
changesets/packages/cli/src/commands/bump/index.js
Lines 139 to 141 in 5d03edd
// TODO: this error message was copied directly from bolt | |
// it seems wrong, shouldn't this case be covered by dependents stuff | |
// and this should be something like "this should never happen, please open an issue because there's probably a bug in changesets" |
* Remove usages of bolt * Fix the cli asking for an otp multiple times incorrectly
No description provided.