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 merge woe with new K/V trait function #1524

Merged
merged 1 commit into from
May 19, 2023

Conversation

itowlson
Copy link
Contributor

K/V validation added a new StoreManager trait function. Unfortunately, in the meantime, we had added a new StoreManager implementation (Azure CosmosDB). And I didn't think about this when I merged (because there was no conflict). And now main is broken.

So, uh, this unbreaks main.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@rajatjindal
Copy link
Collaborator

hmm wondering how come our CI didn't catch this issue during the PR?

@itowlson
Copy link
Contributor Author

@rajatjindal I think because:

  1. I based the validation branch off main a few days ago.
  2. After the last commit to the validation branch, and after all checks were complete, Cosmos merged.
  3. Cosmos did not touch the same files as the validation branch, so GitHub saw no conflict, and left the validation PR checks as green.
  4. I did not rebase the validation branch before merging, so GH had no prompt to re-run the PR checks.

I am not sure if there is a reasonable way we can force checks to re-run on changes to what GH sees as unrelated parts of the code... even if we can force checks, it could get very noisy and inefficient without some smarts to it. I'm not very knowledgeable about what we can do in this area though.

@itowlson itowlson merged commit 8dcbad0 into fermyon:main May 19, 2023
@rajatjindal
Copy link
Collaborator

that makes sense. thank you for explaining.

@rylev
Copy link
Collaborator

rylev commented May 19, 2023

I think this is a good example of why we run CI on main after merge. The only way to prevent this is to have a merge queue that rebases the PR on top of main, runs CI, and then merges. This would guarantee that main is always in a good state.

@lann
Copy link
Collaborator

lann commented May 19, 2023

Yeah, logical conflicts are an inherent problem with this workflow. For large projects with slow CI and tons of commits every day you really care about this but I think we can just deal with the occasional main breakage.

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.

4 participants