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

Remove elmah table usage from gallery #10021

Merged
merged 7 commits into from
Jun 24, 2024
Merged

Remove elmah table usage from gallery #10021

merged 7 commits into from
Jun 24, 2024

Conversation

drewgillies
Copy link
Contributor

Partially addresses: https://github.com/NuGet/Engineering/issues/5480

We'll remove this table's usage as a part of the SDK migration effort.

v-manil2 and others added 2 commits May 28, 2024 23:25
@drewgillies drewgillies requested a review from a team as a code owner June 13, 2024 04:21
@drewgillies
Copy link
Contributor Author

FYI @Lanaparezanin

erdembayar
erdembayar previously approved these changes Jun 13, 2024
Copy link
Contributor

@erdembayar erdembayar left a comment

Choose a reason for hiding this comment

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

One less dependency 👏

@drewgillies drewgillies removed the Draft label Jun 17, 2024
@joelverhagen
Copy link
Member

PR against dev please

@@ -8,13 +8,10 @@
namespace NuGetGallery.Diagnostics
{
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true)]
public sealed class SendErrorsToTelemetryAttribute
: ElmahHandleErrorAttribute
public sealed class SendErrorsToTelemetryAttribute : Attribute
Copy link
Member

Choose a reason for hiding this comment

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

Does this still get invoked without Elmah installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch--I'll remove this.

@@ -295,7 +293,7 @@ private static void BackgroundJobsPostStart(IAppConfiguration configuration)
{
RestartSchedulerOnFailure = true
};
_jobManager.Fail(e => ErrorLog.GetDefault(null).Log(new Error(e)));
_jobManager.Fail(e => { /* fail silently */ });
Copy link
Member

Choose a reason for hiding this comment

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

It may be possible to use Trace here. I believe we have an App Insights trace listener. It might be worth having this "just in case".

@joelverhagen
Copy link
Member

Love the clean-up!

joelverhagen
joelverhagen previously approved these changes Jun 17, 2024
@drewgillies drewgillies changed the base branch from main to dev June 17, 2024 21:58
@drewgillies drewgillies dismissed stale reviews from joelverhagen and erdembayar June 17, 2024 21:58

The base branch was changed.

erdembayar
erdembayar previously approved these changes Jun 18, 2024
@agr
Copy link
Contributor

agr commented Jun 18, 2024

Should it include a change to admin index page to remove Elmah link?

agr
agr previously approved these changes Jun 22, 2024
@drewgillies drewgillies marked this pull request as draft June 24, 2024 10:10
@erdembayar
Copy link
Contributor

erdembayar commented Jun 24, 2024

I'm wondering why Elmah.dll was reintroduced to sign.thirdparty.props?
Is it still used somewhere?

@drewgillies drewgillies marked this pull request as ready for review June 24, 2024 21:36
@drewgillies drewgillies merged commit b1f57a8 into dev Jun 24, 2024
2 checks passed
@drewgillies drewgillies deleted the dg-elmah branch June 24, 2024 22:01
@drewgillies
Copy link
Contributor Author

drewgillies commented Jun 25, 2024

@erdembayar @joelverhagen the file being unsigned broke the build, and it persists in the build to support Elmah migrations (backward and, in future, forward [edit: correction--there was no backward migration to remove table/sprocs, and I doubt we'll add it--forward would be a good cleanup step though]). I will review this in separate work to see if it can be removed without removing migration support. See https://github.com/NuGet/Engineering/issues/5495 for tracking.

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.

6 participants