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

Fixed bug where corral update would result in incorrect code in the corral #194

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

SeanTAllen
Copy link
Member

When a dependency had a version constraint rather than a single value,
the first time `corral update` was run, you wouldn't end up with the
correct code checked out. The constraint was correctly solved, but the
checked out code would be for branch `main`.

The problem arose because we first have to check something out and then
can do constraint solving.

This commit makes the change to keep track of what "something" was that
was initially checked out and if it differs from what we eventually
determine the revision should be, we run another update to get the
contents of `_corral` into the correct state.

@SeanTAllen SeanTAllen requested a review from a team July 19, 2021 03:27
@SeanTAllen
Copy link
Member Author

This builds on #193. Once #193 is merged, I'll rebase this against main and it can then be merged.

@SeanTAllen SeanTAllen added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge do not merge This PR should not be merged at this time labels Jul 19, 2021
@SeanTAllen
Copy link
Member Author

I think we can ignore the MacOS failures.

You can see here: 2e72800 that I added MacOS CI just a couple days ago. In all likelihood, given that Windows and Linux passed, this change is fine but there's an underlying issue with the failing tests on MacOS or a MacOS bug to track down.

I'll work on figuring out that source of bug (like the windows CI bug I finally tracked down and fixed last week that made Windows CI reliable as compared to failing 99% of the time on a couple of the integration tests).

@SeanTAllen SeanTAllen changed the title Fixed bug where checked out code not matching revision Fixed bug where corral update would result in incorrect code in the corral Jul 19, 2021
@SeanTAllen SeanTAllen force-pushed the update-finishes-correct branch from f153f97 to aef390a Compare July 19, 2021 12:55
@SeanTAllen
Copy link
Member Author

So the failure earlier I can't reproduce now. It was a timeout so we might with the bit of additional work that is being done, increase the test timeouts for MacOS, but I will monitor the situation and see.

@jemc
Copy link
Member

jemc commented Jul 20, 2021

#193 has been merged.

When a dependency had a version constraint rather than a single value,
the first time `corral update` was run, you wouldn't end up with the
correct code checked out. The constraint was correctly solved, but the
checked out code would be for branch `main`.

The problem arose because we first have to check something out and then
can do constraint solving.

This commit makes the change to keep track of what "something" was that
was initially checked out and if it differs from what we eventually
determine the revision should be, we run another update to get the
contents of `_corral` into the correct state.
@SeanTAllen SeanTAllen force-pushed the update-finishes-correct branch from aef390a to f2e70d7 Compare July 20, 2021 23:41
@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Jul 20, 2021
@SeanTAllen
Copy link
Member Author

This is ready for review/merging.

@jemc jemc merged commit 4010cbe into main Jul 21, 2021
@jemc jemc deleted the update-finishes-correct branch July 21, 2021 15:33
github-actions bot pushed a commit that referenced this pull request Jul 21, 2021
github-actions bot pushed a commit that referenced this pull request Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants