Skip to content
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

fix: prevent concurrency issue whene multiple channel are released #2077

Conversation

KillianHmyd
Copy link
Contributor

This would reduce the risk of conflicts error when two different channels are released at the same time.

Because at each release for every channel a new note is pushed on refs/notes/semantic-release, sometimes we have the following error :

To https://github.com/PayFit/subscriptions.git
 ! [rejected]        refs/notes/semantic-release -> refs/notes/semantic-release (fetch first)
error: failed to push some refs to 'https://github.com/PayFit/subscriptions.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

A fetch before adding a new note would reduce this risk.

@gr2m
Copy link
Member

gr2m commented Aug 13, 2021

Could you add a test please?

@KillianHmyd KillianHmyd force-pushed the fix/concurrency-issue-multiple-channels branch from 29a42e6 to dbdce12 Compare August 16, 2021 08:07
@KillianHmyd
Copy link
Contributor Author

@gr2m I fixed the code, and so the tests, but not sure which tests should I add. As I only do a fetch note before pushing them, the only solution to test this concurrent issue would be to add mocks over the git helpers which doesn't seems to be a thing in this project 🤔

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fetch before adding a new note would reduce this risk.

could you verify that the patch is resolving the conflicts in your environment?

@@ -112,6 +122,7 @@ async function run(context, plugins) {
if (options.dryRun) {
logger.warn(`Skip ${nextRelease.gitTag} tag creation in dry-run mode`);
} else {
await fetchNotes(options.repositoryUrl, {cwd, env});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment on why we fetch the notes here, with a reference to the pull request?

@KillianHmyd
Copy link
Contributor Author

@gr2m Sorry for the latency on this PR.
So we tested it from our side and it resolves only partially our conflicts. In our case we release using semantic release on each branches, so even if it lower the risk of conflicts we still have some.

I thought about another solution but which would introduce a breaking change. The solution would be to use a dynamic note reference, so instead of pushing them on semantic-release ref, do it on semantic-release-${TAG_REFERENCE}.
This way each tag would have their own note references and it would prevents conflicts between two unrelated releases.

It could be possible to keep retro-compatibility by fetching notes from the new dynamic reference and the current static one.

I can open a ticket or a WIP PR to discuss about it if you think it is relevant 😃

@KillianHmyd
Copy link
Contributor Author

Closed for this one : #2085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants