Added check for ALLOWED_CONTENT_CHECKSUMS that Artifacts are missing

[daviddavis]

fixes #7487

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

[pulpbot]

Attached issue: https://pulp.plan.io/issues/7487

[ipanova]

⚡

[mdellweg]

I'd prefer if we could be more specific about the exception we want to pass. We would not need to reraise ImproperlyConfigured in that case.
Also is it possible, that connection.close() raises an error?

[daviddavis]

I'd prefer if we could be more specific about the exception we want to pass. We would not need to reraise ImproperlyConfigured in that case.

My thought about catching a generic Exception is that there's a wide variety of different exceptions that could be raised. In most cases, I think we just want to forgo checking ALLOWED_CONTENT_CHECKSUMS and not raise ImproperlyConfigured (which pass should accomplish).

Also is it possible, that connection.close() raises an error?

This is possible. I will add a try/except.

[bmbouter]

Moving the connection.close() into the existing try/except I think would make sense.

[daviddavis]

I can do that. I didn't put the connection.close() in the try block because I assumed that it might not be reached if an exception gets thrown and thus the connection might not get closed properly. But I guess at that point, if an exception gets thrown, the process is mostly likely going to exit anyway?

[mdellweg]

Jeps!

[bmbouter]

So will this work out so well for us? We had to add this explicit close because not having it was secretly leaving a connection open. If we silence an error here and continue we're back in that situation. If there is a problem to the extent that the close() call fails, I think it would be better for users to know that and have that be a fatal exception for Pulp.

[daviddavis]

It depends. I think the problem with not catching close() exceptions is that we might be halting the code in cases where we actually want the code to proceed. I did some more testing though and it looks my previous statement about close() was incorrect. close() looks like it doesn't tend to raise exceptions in cases where connection.cursor() does (eg there's no db initialized, bad db password, etc). So I think not rescuing here is probably the correct choice.

[bmbouter]

I think the code as-is is the strongest, but if @mdellweg and @daviddavis, you'd like to do it differently feel free to adjust. I'm leaving the lgtm I put on it with support for either approach.

[mdellweg]

@daviddavis thanks that is a good analysis and probably the right answer to my question.
@bmbouter I'm retiring my concerns.

[bmbouter]

Looks good, thank you!

[ipanova]

the change looks good to me however i don't understand why travis fails.

I have read on your scrum and seen that dynaconf had a bug