Skip to content

Add a log message to add_crate background job #2231

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

Merged
merged 6 commits into from
Mar 1, 2020

Conversation

rye
Copy link
Contributor

@rye rye commented Feb 26, 2020

Per #2226; is this sufficient to resolve that issue?

This PR simply adds a log message (println!) right before carrying out the final commit_and_push operation, which could otherwise fail silently if the Git host is experiencing an outage.

See rust-lang#2226.  This commit simply adds a log message before carrying out
the final commit_and_push operation, which could fail if the Git host is
experiencing an outage.

Signed-off-by: Kristofer Rye <[email protected]>
Tested-by: Kristofer Rye <[email protected]>
@rust-highfive
Copy link

r? @jtgeibel

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Member

@jtgeibel jtgeibel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind tweaking the implementation as described below? It would be great to do the same thing with the yank/unyank message.

Finally, I suggest renaming commit_and_push to commit_and_push_inner, and replacing it with a wrapper function that prints a success message, or the error string. That way we will have a start and end timestamp in our logs for all index operations.

@rye
Copy link
Contributor Author

rye commented Feb 29, 2020

I think I may just try moving the println! statement(s) into the commit_and_push method, since in all invocations we pass a Git commit message that describes what the commit does. Then we don't need a wrapper function; we just print out one (or two) messages containing the commit message and that'll probably be sufficient.

rye added 2 commits February 29, 2020 07:43
This is passed directly into the commit_and_push method in the second
case, but this aligns the practices between the two primary background
job operations associated with the Git repository.

Signed-off-by: Kristofer Rye <[email protected]>
One of the nice things about doing it this way is that it'll be possible
to e.g. time the push operations now, allowing to figure out how much
time is spent either preparing (adding/committing) or pushing, and to
diagnose if operations are timing out or not.

Signed-off-by: Kristofer Rye <[email protected]>
@rye rye requested a review from jtgeibel February 29, 2020 13:56
Copy link
Member

@jtgeibel jtgeibel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more suggestions. Thanks for the updates so far @rye.

rye added 3 commits March 1, 2020 09:57
I liked having it for timing purposes, but it's a bit superfluous and
(understandably) adds a lot of complexity, given the number of times
that this callback will be run.

Signed-off-by: Kristofer Rye <[email protected]>
Now, the wrapper prints the message before the commit and push begins,
and proceeds to either print a success message or eprint the error
before yielding it back.

Signed-off-by: Kristofer Rye <[email protected]>
@rye rye requested a review from jtgeibel March 1, 2020 16:26
@jtgeibel
Copy link
Member

jtgeibel commented Mar 1, 2020

LGTM, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2020

📌 Commit c74d657 has been approved by jtgeibel

@bors
Copy link
Contributor

bors commented Mar 1, 2020

⌛ Testing commit c74d657 with merge e566283...

@bors
Copy link
Contributor

bors commented Mar 1, 2020

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing e566283 to master...

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