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

Pull request not created when using wlc #12400

Open
2 tasks done
rix0rrr opened this issue Sep 2, 2024 · 9 comments
Open
2 tasks done

Pull request not created when using wlc #12400

rix0rrr opened this issue Sep 2, 2024 · 9 comments

Comments

@rix0rrr
Copy link

rix0rrr commented Sep 2, 2024

Describe the issue

In order to prevent merge conflicts, we use automation workflows that use wlc to get Weblate to create PRs. Our workflow looks like this:

  • Use wlc repo to check if there are changes, stop if there aren't any
  • wlc lock all components
  • wlc pull
  • wlc commit
  • wlc push
  • wlc reset to force the repository back to a clean state, because the PR will be squash-merged and otherwise we would get merge conflicts on the next pull

When the PR is merged, a workflow runs automatically in the on_close event:

  • wlc unlock all components again

We notice that occasionally, even though the repository shows needs_commit: True, after running:

wlc commit
wlc push

No PR is created after all. Since no PR is created, our unlock workflow has no chance to run and the Weblate translations remain locked forever.

In the most recent case it was even worse: no PR was created, and as I mentioned wlc reset was run so all new translations were lost 😞 I figured this was safe because after running commit and push, all changes would end up in the PR, but it turns out that sometimes no PR is created at all.

A timestamp when this happened is 2024-09-01 06:15:32 UTC, on the Hedy project: changes were pending, but running commit and push didn't create a PR.

This is not consistent behavior. If I'd have to guess, I would say it happens in ~10% of cases... but with changes every day it happens often enough that it's quite disruptive for us.

What is the reason this happens? How can we write a script using wlc that will reliably create a PR if there are changes to commit?

I already tried

  • I've read and searched the documentation.
  • I've searched for similar filed issues in this repository.

Steps to reproduce the behavior

Write a script using WLC that looks like this:

wlc pull
wlc commit
wlc push

Make changes to a Weblate translation, and run the script above.

In about ~10% of cases, no PR is created.

Expected behavior

I would expect a PR to be created in every case.

Screenshots

No response

Exception traceback

No response

How do you run Weblate?

weblate.org service

Weblate versions

No response

Weblate deploy checks

No response

Additional context

No response

rix0rrr added a commit to hedyorg/hedy that referenced this issue Sep 2, 2024
It seems that `wlc commit` followed by `wlc push` doesn't always create
a PR (WeblateOrg/weblate#12400). This is the cause of the Weblate
repository sometimes being locked without ever getting unlocked:
unlocking happens on the PR-close workflow, but the PR never gets
created.

In fact, since we also run a `wlc reset` to avoid future merge conflicts,
the `wlc reset` destroys all pending translator changes, which has
happened to some contributors and now their work is gone.

Add some sleeps to get a (presumably) async commit process to finish,
and do an additional check for the `push` flag to reduce chances
of data loss.
mergify bot pushed a commit to hedyorg/hedy that referenced this issue Sep 2, 2024
It seems that `wlc commit` followed by `wlc push` doesn't always create a PR (WeblateOrg/weblate#12400). This is the cause of the Weblate repository sometimes being locked without ever getting unlocked: unlocking happens on the PR-close workflow, but the PR never gets created.

In fact, since we also run a `wlc reset` to avoid future merge conflicts, the `wlc reset` destroys all pending translator changes, which has happened to some contributors and now their work is gone.

Add some sleeps to get a (presumably) async commit process to finish, and do an additional check for the `push` flag to reduce chances of data loss.

**How to test**

This is hard to test in isolation. We'll just have to monitor the GitHub Workflow runs over the next couple of days.
@nijel
Copy link
Member

nijel commented Sep 3, 2024

commit triggers background creation of the pull request (if push on commit is turned on), so push should not be needed. But there is no guarantee that it is started before reset. IMHO the reset should be executed upon pull request merge, not when creating it (Weblate is locked meanwhile anyway).

Do you have a log for the failed run? There might be something useful in wlc output there.

Moreover, you might want to use squashing at Weblate side to avoid reset completely (but that might need other workflow adjustments).

@rix0rrr
Copy link
Author

rix0rrr commented Sep 3, 2024

"push on commit" is not turned on, because sometimes Weblate will commit of its own accord (and hence push), and I only want the PR created at defined times of the day.

Is the handling of wlc commit also asynchronous?

Do you have a log for the failed run? There might be something useful in wlc output there.

I did not run it with --debug, so I don't have a log on my end, just a timestamp. I was hoping you would have some server-side logs to look at.

Moreover, you might want to use squashing at Weblate side to avoid reset completely (but that might need other workflow adjustments).

Even if Weblate squashed on its end, it's unlikely it would come up with the exact same commit as GitHub would produce (same author, same commit message, etc). Yes we could regular-merge the pre-squashed commit, but it would need special scripting to handle Weblate PRs differently from other PRs (which are always squashed), and in any case we might need to do some final tweaks to Weblate PRs because we might need to adjust translations that have broken source code in them.

So there is really no way of avoiding the reset, I don't think.

IMHO the reset should be executed upon pull request merge

That's an idea! I suppose you're right that it shouldn't matter from the Weblate PoV. But I wonder if we'd run into an ordering problem there: the reset needs to be run before the PR is merged, because merging the PR updates the main branch, which Weblate would then pull and it would discover merge conflicts.

So we'd need the following steps in this order:

  • Weblate is locked
  • PR is created
  • ....
  • PR is approved
  • Weblate is reset
  • PR is merged
  • (Weblate auto-pulls)
  • Weblate is unlocked

I'm not sure I can confidently script those events in GitHub.

@nijel
Copy link
Member

nijel commented Sep 3, 2024

If you use rebase in git, it will look at the content of the commit and will discards commits with the same content. So it doesn't really matter how the commit messages or authors look like.

Reset after merging the pull request is safe, even if there would be a merge conflict, it would be then reset to match upstream.

@rix0rrr
Copy link
Author

rix0rrr commented Sep 3, 2024

Reset after merging the pull request is safe, even if there would be a merge conflict, it would be then reset to match upstream

Yes, but would the order not go like this:

  1. PR is merged, leading to conflicts
  2. Weblate will detect conflicts during auto-pull, and send an email saying "there are merge conflicts"?
  3. Reset is run to discard Weblate state

?

If so, we'd have an email every day warning us about a non-problem, hiding the cases when there's an actual problem.

@rix0rrr
Copy link
Author

rix0rrr commented Sep 3, 2024

If you use rebase in git, it will look at the content of the commit and will discards commits with the same content

I know. That check will fail if we happened to mutate the PR while merging, which can happen because the translations contain source code which might contain problems that need to be fixed.

It's a best-effort check that can fail, which is why I don't want to rely on it.

@rix0rrr
Copy link
Author

rix0rrr commented Sep 3, 2024

Can you please tell me whether the handling of wlc commit is synchronous or asynchronous?

Thanks!

@rix0rrr
Copy link
Author

rix0rrr commented Sep 6, 2024

One more question. If I do the following:

wlc commit
sleep 30
wlc push
sleep 60
wlc repo

I see the pull request being created.

Is it expected that I still see needs_push: True in response to the wlc repo command?

rix0rrr added a commit to hedyorg/hedy that referenced this issue Sep 9, 2024
We used to use `wlc repo` to try and see if Weblate flushed its
changes to a PR.

Weblate always shows `needs_push: True`, even if it *did* create a PR,
so that is not a reliable check.

Instead, we use the GitHub CLI to check for a PR with the title that
Weblate always creates. We already check for the absence of such a PR at
the start of the script, so if a PR exists at the end of it, it must be
because Weblate created it, and we can safely reset.

Context: WeblateOrg/weblate#12400
mergify bot pushed a commit to hedyorg/hedy that referenced this issue Sep 9, 2024
We used to use `wlc repo` to try and see if Weblate flushed its changes to a PR.

Weblate always shows `needs_push: True`, even if it *did* create a PR, so that is not a reliable check.

Instead, we use the GitHub CLI to check for a PR with the title that Weblate always creates. We already check for the absence of such a PR at the start of the script, so if a PR exists at the end of it, it must be because Weblate created it, and we can safely reset.

Context: WeblateOrg/weblate#12400
@nijel
Copy link
Member

nijel commented Sep 13, 2024

Right now wlc commit is synchronous. The possible triggered push is not.

I'm not sure about needs_push semantic right now. It might be also that the changes needs to be merged upstream. The flag is there from times when pushing was to the same branch and repo and its semantic was clearer back then.

@rix0rrr
Copy link
Author

rix0rrr commented Sep 13, 2024

Got you, thanks.

I've changed to using the GitHub API to see if a PR was created.

I also noticed that there can be quite a delay between an upstream repository change and Weblate pulling the changes.

We had an interval of ~45 minutes the other day. Of course, a translator had translated in the interval between us merging the PR and unlocking Weblate, and Weblate actually performing the pull, so we had another merge conflict.

Do you have statistics on typical values for the notification-pull delays?

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

No branches or pull requests

2 participants