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

Always write the update output file, even on container failure #376

Conversation

rhyskoedijk
Copy link
Contributor

@rhyskoedijk rhyskoedijk commented Oct 23, 2024

If there are successful update outputs, it would be useful if Dependabot wrote them to the output file so that the caller can process them, even if the overall update process encountered errors.

This was encountered when testing dependabot/dependabot-core#10834 and also in the tinglesoftware/dependabot-azure-devops implementation of Dependabot for Azure DevOps, see tinglesoftware/dependabot-azure-devops#1407 (comment).

When this happens...

updater | +------------------------------------------------------------------------------------------------------------------------------------+
updater | |                                                Changes to Dependabot Pull Requests                                                 |
updater | +---------+--------------------------------------------------------------------------------------------------------------------------+
updater | | created | Microsoft.Extensions.DependencyInjection ( from 8.0.0 to 8.0.1 ), Microsoft.Extensions.Logging ( from 8.0.0 to 8.0.1 ... |
updater | +---------+--------------------------------------------------------------------------------------------------------------------------+
updater | Dependabot encountered '1' error(s) during execution, please check the logs for more details.
updater | +-----------------------------------------------------------------+
updater | |                  Dependencies failed to update                  |
updater | +-------------------------------------------+---------------------+
updater | | Microsoft.Extensions.Logging.Abstractions | update_not_possible |
updater | +-------------------------------------------+---------------------+

The "create_pull_request" output for the successful update could still be processed by the caller, if CLI wrote the output file.
After this change, the outputs from a partially successful Dependabot update can still be processed whilst also still reporting that the overall update failed.

@rhyskoedijk rhyskoedijk requested a review from a team as a code owner October 23, 2024 04:33
@rhyskoedijk
Copy link
Contributor Author

@jakecoffman @JamieMagee @brettfo sorry to ping you directly, but I've noticed you're active here recently; Is there any chance this PR would be considered? I'm facing a roadblock due to dependabot/dependabot-core#10834 and this change would really help keep the PRs flowing.

@brettfo
Copy link
Contributor

brettfo commented Oct 29, 2024

...sorry to ping you directly...

Don't be sorry, this is exactly why we're getting paid.

As for the change, I'm good with it and it makes sense from my perspective, but I mostly focus on the NuGet updater, so I'll yield to @jakecoffman on this one.

@JamieMagee
Copy link
Contributor

We'll likely not be able to merge until later this week, or early next week. We're pausing while GitHub Universe is on.

Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

This seems reasonable, thanks!

You might consider using the data from stdout or using api-url to get the HTTP requests directly from the Updater container. This will allow you to process the message immediately instead of waiting until the end, and avoids any kind of processing issues like the one this PR fixes.

@jakecoffman jakecoffman added this pull request to the merge queue Oct 31, 2024
Merged via the queue into dependabot:main with commit 6aa84ce Oct 31, 2024
73 checks passed
@rhyskoedijk rhyskoedijk deleted the feature/write-output-file-on-update-error branch October 31, 2024 19:46
@rhyskoedijk
Copy link
Contributor Author

You might consider using the data from stdout or using api-url to get the HTTP requests directly from the Updater container

Good idea, I will look in to this as an improvement. thanks for the help.

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