greenkeeperio / greenkeeper-lockfile

:lock: Your lockfile, up to date, all the time
https://greenkeeper.io
183 stars 73 forks source link

Fix firstPush when uploading lockfiles as part of a Circle 2.0 workflow #118

Closed nevir closed 6 years ago

nevir commented 6 years ago

Circle 2.0 workflows specify CIRCLE_PREVIOUS_BUILD_NUM for builds in the same workflow, so it's unfortunately pretty useless in that case


Example build where it was failing previously: https://circleci.com/gh/convoyinc/apollo-cache-hermes/3368 Notice that build 3367 ($CIRCLE_PREVIOUS_BUILD_NUM) is part of the same workflow: https://circleci.com/workflow-run/f51c1998-e980-4b4c-9a1f-ebdc2b917419

patkub commented 6 years ago

Whichever job runs first in the workflow will get CIRCLE_PREVIOUS_BUILD_NUM = false. So if you run the greenkeeper job first (and make sure it is always first), not last, in your workflow it should work. See https://github.com/greenkeeperio/greenkeeper-lockfile#circleci-workflows

With your changes, if you (accidentally) run greenkeeper-lockfile in two jobs that run in parallel in a workflow, you could run into a race condition. Both jobs will have 1 commit on the branch, and both could trigger firstPush. The current check that uses CIRCLE_PREVIOUS_BUILD_NUM ensures that only one job will ever trigger firstPush, but restricts its use to the first job in the workflow.

janl commented 6 years ago

siding with @patkub here and closing. re-open if you have more evidence :)

patkub commented 6 years ago

Edit: It didn't re-open.

Re-opening as I found something relevant. The upload task checks for firstPush and then uploadBuild. But, the update task already checks for firstPush. Why does the upload task also need to check firstPush? Is the uploadBuild check not enough?

Removing the firstPush check from the upload task would let the user upload the lockfile at the end of the workflow, and ensure the update task only commits once.

For CircleCI the upload build check is https://github.com/greenkeeperio/greenkeeper-lockfile/blob/258e9c0f2eae7eb68321da7d6fd877e565a30302/ci-services/circleci.js#L12

nevir commented 6 years ago

Isn't the intention of greenkeeper-lockfile-upload to only be run after a successful test run? If so, we need to be able to run that outside of the first workflow step