Support branches other than "master" - #175
Conversation
|
@confused-Techie Any feedback? |
|
Sorry! Can't speak for @confused-Techie, but I know that I've been busy with other things. Poking us occasionally is 100% the right thing to do. |
|
@confused-Techie Any news on this? |
|
@savetheclocktower Can you take a look? Will #172 solves the issue? |
savetheclocktower
left a comment
There was a problem hiding this comment.
Tests pass for me locally and the code looks fine. One observation that is not actionable right now.
@idleberg, have you been able to publish packages using your modified version of ppm? That's the only other thing I would consider testing — but you've contributed before and if you've managed to prove it works, that'd be good enough for me.
Gonna approve now and land soon (probably tomorrow; will set a reminder) just in case @confused-Techie wants to take a look.
| const ref = execSync('git symbolic-ref refs/remotes/origin/HEAD', { | ||
| encoding: 'utf8', | ||
| stdio: ['pipe', 'pipe', 'ignore'] | ||
| }).trim(); |
There was a problem hiding this comment.
This is the first place in ppm we'd be calling execSync just to read some information. That doesn't mean I'm against it — but it does mean we should probably add something to git-utils to allow reading the same information more programmatically.
This will do in the meantime — especially since there are thorough specs.
There was a problem hiding this comment.
That makes sense to me. I investigated some more and learned that npm uses @npmcli/git for its git operations. However, that delegates processes to @npmcli/promise-spawn, so the signature of the method would change and become async. Changing this would likely become a rabbit hole that should be avoided in the scope of this PR.
I could imagine creating a new git module within ppm, but I no preference whether to involve a third-party library. Generally speaking, maybe it's best to weigh in some more opinions, if you prefer to.
There was a problem hiding this comment.
While I had momentum on this I added some APIs to git-utils; feel free to play around with this PR and see if it would do what you need.
confused-Techie
left a comment
There was a problem hiding this comment.
Sorry for the massively delayed response.
But overall, the idea is sound, code looks clean, and with the amount of tests added to the publishing suite I'm inclined to trust it.
If all of @savetheclocktower's comments are fully satisfied I see no reason we can't merge this in and finally get this updated. Thanks for the hard work!
|
@idleberg, |
ppm still uses |
|
Whoops. Ordinarily I'd tell you use to use the So, yeah, I'd say it's safe to move entirely to |
|
The tests still fail on NodeJS 14 and 16, |
|
Might as well do it in this PR and add 20 to the matrix. (You've been patient enough and I won't make you put another blocker in the way!) |
|
The new failures are strange and I can only assume this is a expect(repo.isWorkingDirectory(workingDirectory.toUpperCase())).toBe(true)Let me do some quick investigation. Sorry this has been such a pain! |
|
OK, I know what's going on. When I updated to newer But the spec I mentioned before (for Currently testing a fix in CI; if it works, I'll fast-track a PR on the |
|
@idleberg, I'm releasing you from this experiment. I am making progress over on Once it ships, I'm happy to revisit this and bump |
d370d7f to
fee3e22
Compare
This reverts commit bf033cb.
fee3e22 to
1714a50
Compare
I've reverted the two commits adding |
|
OK, you can leave Node 22 out of the matrix. :) (This puts a deadline on migrating to |
|
Landing this. Thanks, @idleberg, for shepherding this PR through our unreasonably long gauntlet! |
GitHub has changed the default branch from
mastertomainin October 2020, setting an example that has since been followed by its competitors. However, Atom/Pulsar packages are still stuck at usingmaster. Publishing a package from a different branch will fail.This PR addresses this twofold:
git symbolic-ref--branchflag