Skip to content
This repository was archived by the owner on Jul 2, 2021. It is now read-only.

Conversation

powerjg
Copy link
Member

@powerjg powerjg commented Aug 10, 2020

Signed-off-by: Jason Lowe-Power [email protected]

@powerjg powerjg requested a review from hnpl August 10, 2020 18:35
If the artifact can't be found in the db when we're trying to
initialize it with a UUID, add a bunch of fake defaults to make other
callers (e.g., run.loadFromDict) happy.

Signed-off-by: Jason Lowe-Power <[email protected]>
@powerjg
Copy link
Member Author

powerjg commented Aug 10, 2020

Testing is cool! The static analyzer caught one problem that was non-obvious (and another obvious problem that I was too lazy to test :/)

@powerjg powerjg changed the title artifact: Add clearer error if artifact is missing from db artifact: Initialize if missing from db Aug 10, 2020
@hnpl
Copy link
Member

hnpl commented Aug 10, 2020

I wonder in what scenario we cannot retrieve the Artifact's, and whether the fake defaults will ever be written to the database.

My understanding is that, the only time that an Artifact being written to the database is when we call RegisterArtifact(). In that case, all information is not garbage, and other functions only read from the database. So, creating an Artifact using init won't result in fake defaults being written to the database.

If that is the case then I think this makes sense.

Other than that, I think we should output some warnings as well when we cannot retrieve the Artifact from the database. We don't want it to be silently failed.

@powerjg
Copy link
Member Author

powerjg commented Oct 1, 2020

Even though the underlying problem (someone deleting everything from the database) was different, I still think this change is useful. However, I can see how there may be unintended side effects.

@takekoputa, how do you suggest we have warnings?

I almost just feel like leaving this PR open and coming back to this if we find another use case (other than being ransomwared ;))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants