Dry Run works but running for dies silently and deletes package.json

[jbcpollak]

I'm running release publish on a none-Node repo, so I've created a stub package.json. When I run the command with -d, everything runs as I'd expect. But when I run without -d, it dies immediately after listing out all the commits.

The output starts out:

INFO: preparing release for "xxxx" from branch "main"...
INFO: found no previous releases, creating the first one...
INFO: found 24080 new commits:

then it lists and the commits, then dies. it also deletes package.json, which is pretty confusing.

The contents of package.json are:

{
  "name": "@myorg/myrepo",
  "description": "Does a thing.",
  "version": "0.0.0",
  "author": {
    "email": "[email protected]",
    "name": "The team at My Org",
    "url": "http://www.myorg.com"
  }
}

I took a look at bumpPackageJson.ts and I don't see how it could actually delete the file. 🤷

[jbcpollak]

ok, I think I figured out what is happening. I ran it again and finally caught some logs:

INFO: successfully parsed commits: 24080
INFO: next release type: minor
INFO: next version: 0.0.0 -> 0.1.0
INFO: bumped version in package.json to:
INFO: preparing to run the publishing script with:
{"RELEASE_VERSION":"0.1.0"}
INFO: executing publishing script...
INFO:
INFO: published successfully!
INFO: created a release commit at "432568d4febcd58b8ed1b71e739f5df364175eaf"!
ERROR: Failed to tag the release!
 Error: Command failed: git tag v0.1.0
fatal: tag 'v0.1.0' already exists

ERROR: release failed, reverting changes...
INFO: reverting the release commit...

I think two things are happening:

the release fails to tag (this actually isn't unexpected, I don't know why the tag exists, but I don't actually have permission to push to the main branch, so I was expecting some sort of failure here.

The package.json is being deleted because its brand new and never committed, so release is using git to revert it into nothingness.

[kettanaito]

Oh, a nice find!

So we do validate the GitHub token you provide but I don't think we check if you have permissions to push as a preliminary step. It'd be nice to have that. I believe Semantic Release does a similar check, we can look into how they achieve it. It's likely a matter of performing a readonly command that's tied to write permissions.

The package.json is being deleted because its brand new and never committed, so release is using git to revert it into nothingness.

That's not okay. What do you say if we reverted an explicit list of files? We can keep track of all the files Release generates, and then checkout only those files. This should enable use to run Release on a repo with a bunch of uncommitted changes.

Here's the code that drops all the changes if the release fails:

release/src/commands/publish.ts

Lines 286 to 296 in dcfa47c

	if (hasChanges) {
	this.log.info('detected uncommitted changes, stashing...')
	await execAsync('git stash')
	}
	await execAsync('git reset --hard HEAD~1').finally(async () => {
	if (hasChanges) {
	this.log.info('unstashing uncommitted changes...')
	await execAsync('git stash pop')
	}
	})

Generally, another approach we may take is to stash any changes before running the Release. Then, when the release is finished, unstash them. This would also work well semantically because you shouldn't be releasing things that are not in Git. I'd actually prefer this approach because by stashing everything we're effectively getting a guaranteed clean Git state before each release attempt. Then we can just drop all the changes, knowing those changes belong to the Release tool.

[jbcpollak]

hum, I was thinking the reset --hard would delete the untracked package.json but it doesn't, so I'm still not sure how that was happening.

Honestly, I think the best practice here would be to refuse to run release if there are any local changes to the repo at all, ie if git status --porcelain returns anything, the release should fail before it even starts. I've seen and made too many manual release oopsies because we had in progress work accidentally get released, etc.

If you agree with that and add that functionality, it should be safe to just remove the stash/pop from the revert entirely because you'll know that any local changes must have been made by release by definition.

It would also make whatever was deleting my package.json file a moot issue, since release wouldn't have even run until I committed it.