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

bugfix: show push error messages #5065

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

mitchellwrosen
Copy link
Member

Fixes #4152

Screen Shot 2024-06-10 at 10 44 15 AM

This PR fixes the bug in push that essentially caused error messages to flash briefly, then disappear.

The bug was caused by a bad interaction between our normal "output" mechanism (just putting to stdout) and our live-updating output mechanism (which prints "Uploaded N entities..." where N updates in-place).

The fix is to release the console region resource first, then inspect whether the upload failed and short-circuit with an error message if so, rather than short-circuit with an error message while holding on to the console region resource, because the resource sort of internally tracks how large it is, and so printing messages to stdout and then closing it is not really going to visually do the right thing.

@mitchellwrosen mitchellwrosen marked this pull request as ready for review June 10, 2024 15:16
@aryairani
Copy link
Contributor

I want to start capturing all our messages in transcripts. Is there a way to do that for this one?

@mitchellwrosen
Copy link
Member Author

@aryairani I don't think so, it hits the network, and our transcript tests don't have a sort of mock network layer

@aryairani
Copy link
Contributor

I think hitting the network is not a deal-breaker as long as we can get reasonably consistent results.
e.g. we already have transcripts that need to download unison/base.
But agree it doesn't make sense to try to capture every kind of network failure with transcripts.

@aryairani aryairani merged commit c758076 into trunk Jun 10, 2024
20 checks passed
@aryairani aryairani deleted the show-push-auth-error-message branch June 10, 2024 16:06
@mitchellwrosen
Copy link
Member Author

Oh, I didn't realize we already hit the network in transcripts. Which transcripts download @unison/base?

It would be easy to capture this output in a transcript – just try pushing to something that like @unison/base/main and get rejected. I just assumed we wouldn't want to hit the network in our test suite, because it seems like it could be a hinderance in a few ways.

@aryairani
Copy link
Contributor

aryairani commented Jun 10, 2024

Looking at them, it's less clear than I remembered. There's base.md; and then there's also _base.md, which doesn't use the network, but maybe should?

@aryairani
Copy link
Contributor

aryairani commented Jun 10, 2024

I also put some network access in pull-errors.md (relevant / analogous to this PR)

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.

push fails with just ❗️ when authentication is required
2 participants