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

Submodule sync silently failing due to review requirement #167

Closed
ianonavy opened this issue Sep 26, 2022 · 4 comments
Closed

Submodule sync silently failing due to review requirement #167

ianonavy opened this issue Sep 26, 2022 · 4 comments

Comments

@ianonavy
Copy link

As of July 2022, calcom/docker has a branch protection rule on main that requires at least one review before merging. This revealed two bugs:

  1. the submodule sync workflow stopped working
  2. the submodule sync workflow reports a successful job even though it failed

The last step in the Actions workflow for submodule update ends with || echo. Because echo always exit 0, GitHub Actions erroneously believes the job succeeded regardless of whether git push failed due to lack of new commits or failure to push. See example. This has been failing since 1.7.6 cal.com@6b0ac9.

Instead of committing and pushing on one line, I would recommend separating the check for new commits to its own line. That way, you can do an early exit(0) if there are no new commits, and correctly fail the job if git push errors out.

To fix the issue with the branch protection rule, there are a few options. In no particular order:

  1. Remove the PR requirement.
  2. Create a GitHub app as an exception to the branch protection rule (seems complicated, but as of today actions-user cannot be added to the exception list).
  3. Change the sync script to submit a PR instead.
  4. Instead of using a submodule, just do a git clone in the docker build and push workflow.
@krumware
Copy link
Member

Thanks for calling this out! I'll look into it

@ianonavy
Copy link
Author

ianonavy commented Sep 26, 2022

Thanks! For what its worth, I think PR #147 will resolve this by removing the submodule sync.

@krumware
Copy link
Member

I temporarily alleviated the protection requirements in the mean time while we sort that out. Also manually triggered the update/build/push

@ianonavy
Copy link
Author

ianonavy commented Oct 3, 2022

Awesome! Looks like that temporary fix is working. Looking forward to the permanent fix.

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