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

Do not swallow exception, print the message #8928

Merged

Conversation

trejjam
Copy link
Contributor

@trejjam trejjam commented Jan 29, 2024

Debugging of the native helper is extremely painful when you do not provide the context of failure.

@trejjam trejjam requested a review from a team as a code owner January 29, 2024 18:13
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Jan 29, 2024
@trejjam trejjam mentioned this pull request Jan 29, 2024
@trejjam trejjam force-pushed the feature/nuget-do-not-swallow-exception branch from e0745fe to f92328e Compare January 29, 2024 20:26
@trejjam trejjam force-pushed the feature/nuget-do-not-swallow-exception branch from f92328e to fa9d24f Compare January 31, 2024 13:25
@trejjam trejjam force-pushed the feature/nuget-do-not-swallow-exception branch from 7c26114 to 7d45541 Compare January 31, 2024 13:34
@brettfo
Copy link
Contributor

brettfo commented Jan 31, 2024

In production this should never fail, but it might when running locally; is that what you're seeing?

@trejjam
Copy link
Contributor Author

trejjam commented Jan 31, 2024

Yes, I had a problem with the configuration of my environment.

The first was that dotnet was not discoverable, and the second was that the native tool was not authorized against a private repository, which is relatively easy to introduce in many places (ruby, dotnet, remote repository, invalid scope of credentials, ...).

@brettfo
Copy link
Contributor

brettfo commented Jan 31, 2024

The reason for the NuGet.Config mangling is specifically to pass through feed credentials that dependabot knows about, but the updater doesn't.

The most common way for dependabot to be run with a custom feed is via a dependabot.yml file in the repo, but if you need to run the tool locally for testing and use a private feed you can do this (I do this from the Docker development shell):

LOCAL_CONFIG_VARIABLES='[{"type":"nuget_feed","token":"MY_SECRET_TOKEN","url":"https://nuget.example.com/index.json"}]' # add feeds as necessary
bin/dry-run.rb nuget dependabot/dependabot-core # ... other arguments

@brettfo
Copy link
Contributor

brettfo commented Jan 31, 2024

Thanks for indulging me in questions, but yes, doing something with an exception rather than silently swallowing it can only be a good thing.

@trejjam
Copy link
Contributor Author

trejjam commented Jan 31, 2024

@brettfo #8928 (comment) a different PR :)

@trejjam
Copy link
Contributor Author

trejjam commented Jan 31, 2024

About "In production, it should never fail." It did because dependabot does not handle well private repositories with native updater (I experience issue with Azure DevOps - it's handled in a different PR)

@brettfo
Copy link
Contributor

brettfo commented Feb 1, 2024

@brettfo #8928 (comment) a different PR :)

I have too many tabs open to keep track. :)

About "In production, it should never fail." It did because dependabot does not handle well private repositories with native updater (I experience issue with Azure DevOps - it's handled in a different PR)

Can you share more information about these scenarios and how the NuGet.Config is failing to get patched? I can't think of a production scenario that would pass in one instance, but not in the other, because that file is always in the same place.

@trejjam
Copy link
Contributor Author

trejjam commented Feb 1, 2024

It's not about the file not being patched. The try-catch is not guarding Nuget.config patch.

It guards only calling native updater (passed block).

@abdulapopoola abdulapopoola merged commit ff57d5a into dependabot:main Feb 1, 2024
50 checks passed
@trejjam trejjam deleted the feature/nuget-do-not-swallow-exception branch February 1, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants