lockfile updates failing to push when using the amend option

[travi]

i've been noticing that my lockfiles are not up to date on several projects lately, but didnt pay close attention since random churn isn't uncommon with the lockfiles quite yet. however, now that i've finally looked into it, i do see failures like this one in several projects.

it appears to be related to the force push failing for some reason on projects where i've enabled the option to amend rather than adding an additional commit by setting GK_LOCK_COMMIT_AMEND. i thought i've seen that working after my PR to force push was accepted, but i'm having trouble finding any supporting evidence. i must have just accepted the removal of the second commit to mean it was working.

since the output doesn't give much to go on, would you maybe have any suggestions to debug this further? i am really surprised that the --force-with-lease was not the appropriate fix in this case.

[travi]

it looks like the push limits to just the HEAD commit. i'm not very experienced with this approach, but i'm betting this doesn't work with a force-push.

should the HEAD: simply be removed when GK_LOCK_COMMIT_AMEND is set, leaving just the remote branch name? will that cause other issues?

[travi]

@finnp sorry to ping you directly, but since you've given feedback on this feature along the way, i'm hoping you have a few thoughts you could provide.

i'd really like to help move this forward toward a fix, but i'm not quite sure what to try since the error above doesn't give much info. even if i fork and depend on a version with my own guesses, i'd have to wait for a greenkeeper PR to be sent to the project that i updated to even get feedback of whether i'm on the right track.

i've been generating several new projects with this option set, so i'm wondering if waiting to help fix this is the next move i need to make or if i need to start disabling this option on those projects for now

[trevtrich]

Since we are adding a new remote here, I wonder if we'd have to fetch the new remote locally before using force-with-lease since force-with-lease tells it to verify that the local branch sha matches the remote branch sha.

[trevtrich]

Assuming that is actually the problem, I wonder if something like this would give us the protection of --force-with-lease but without first fetching the new remote branches:

git push${process.env.GK_LOCK_COMMIT_AMEND ? ` --force-with-lease=${info.branchName}:origin/${info.branchName}` : ''} gk-origin HEAD:${info.branchName}

[guillaumearm]

I have the same problem on this CircleCI build.

IMO git push --force-with-lease fails because there are 2 differents authors (in my case)
Please take a look to this PR commits.
It's an old successful PR (without the AMEND option).
You can see 2 differents greenkeeper bots : greenkeeper[bot] and greenkeeperio-bot

--force-with-lease is used to avoid erase the work of others, maybe it's the cause.

[travi]

i tried forking and making an update to leverage the suggestion from @richardson-trevor above. it does enable successful pushing of the amended commit, so thanks a ton for that info @richardson-trevor!

however, this results in looping of the builds because greenkeeper-lockfile re-runs on the amended commit, pushing another amend, and triggering another build. i havent yet looked into what is missing as a stop condition for this scenario vs the traditional flow with the extra commit, but clearly there is a bit more still outstanding for the amend feature to be complete.

i'll go ahead and send a PR the change that enables the push, but we really need to get the looping behavior solved as well before the amend feature is actually usable.

[travi]

it looks like the hasLockfileCommit function under src/git-helpers.js simply checks for a commit with the specific message that is traditionally added by greenkeeper-lockfile.

to get the check to work with amends, it could possibly confirm that it has the commit made by the greenkeeper bot, but has both the package.json and package-lock.json instead of just package.json. anyone have other ideas that would be more explicit?

[travi]

Thanks for merging @janl :)

I've been a bit busy since my last reply, so i haven't looked further into updating the checks in hasLockfileCommit. Do you have thoughts on handling that part?

[janl]

@travi parsing by commit message was the simplest thing I could get to work, but if you manage to parse any other identifier, like the committer, we could match for that easily too

[travi]

I'll try to take a look when i get a chance, but I'm worried that having the amend option enabled without that part fixed will now result in a build lol that will never stop. I probably have a few projects or there where i haven't disabled yet, so this would cause some trouble in it's current state.

just wondering if a fix for this might need some additional attention before i can get back to it.

[travi]

just to clarify the issue that remains after that PR was merged, i want to keep this piece visible:

however, this results in looping of the builds because greenkeeper-lockfile re-runs on the amended commit, pushing another amend, and triggering another build.

[travi]

i havent verified that this solves the problem yet, but its looking like if we change

greenkeeper-lockfile/lib/git-helpers.js

Line 44 in e3dc688

exec(`git log --oneline origin/${info.branchName}...${defaultBranch} | grep 'chore(package): update lockfile'`)

to something close to

exec(`git show --oneline --name-only origin/${info.branchName}...${defaultBranch} | grep package-lock.json`)

it should get us pretty close to what we're after

[janl]

This looks fine!

[janl]

Correction, we also have to grep for yarn.lock and npm-shrinkwrap.json, I’ll have a PR up shortly.

[greenkeeperio-bot]

🎉 This issue has been resolved in version 2.3.1 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[travi]

thanks so much for taking this over the finish line, @janl

i'll get it pulled into a few projects to confirm there are no other surprises, but i think this should be the last of them

[janl]

Thanks @travi ^5

[travi]

unfortunately, with v2.3.1, i'm seeing the following error:

fatal: 'gk-origin' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
child_process.js:644
    throw err;
    ^
Error: Command failed: git fetch gk-origin
fatal: 'gk-origin' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.

seems unrelated to the changes for amend, but its preventing me from confirming that our changes fully get this working. this was from a private repo, but if i see it come up for one that is public, i'll post a link in case it provides any more context.

[travi]

@janl since these fixes got merged into what is being released as next, would it make sense to cherry pick these changes back onto latest? if it wouldnt introduce too much trouble on your end, it could be helpful to us to separate this from the monorepo changes that are in flight and appear to have a few kinks to work out.

[janl]

@travi I’d prefer to keep this on master and make @2 latest asap :)

[travi]

Fair enough. Thanks for keeping it moving along. I'll give it another shot today with the latest changes

[travi]

sorry for not following up before now, but i've been distracted with other things after running into what i thought were still unrelated (to the amend related changes) issues with next. however, i upgraded a few other projects to next and just confirmed that the amend is working correctly.

notice that there is only 1 commit, but two changed files (including the lockfile), in this PR

[travi]

@janl any interest in reconsidering pulling this change over to the latest channel since this is now blocked by #164 (comment)?