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

Fixes #22668 - Always delete report records #327

Merged
merged 1 commit into from Mar 9, 2018

Conversation

xprazak2
Copy link
Contributor

No description provided.

@ares
Copy link
Member

ares commented Feb 26, 2018

Does this mean that when user will try to delete it from UI and proxy is down, we'll get into inconsistent state? Could we fix this through foreman-maintain later? If this was found as upgrade issue and we stop deleting reports on proxy which is down, I guess the upgrade can leave leftovers on the proxy?

@xprazak2 xprazak2 force-pushed the proxy-delete branch 2 times, most recently from e07c504 to 1cb3771 Compare February 26, 2018 10:36
@xprazak2
Copy link
Contributor Author

I guess we can keep the logic as it is since this bug affects only reports without policy, which is what the migration is trying to remove.

So we delete the reports without policy just from the db (and we could not delete them even if proxy was online - we need the digest which is stored in policy_arf_report association object, which is likely missing as well)

I would definitely like to have something that would allow us to clean up directories on proxy, I created a ticket in forman_maintain.

else
logger.error "Failed to delete report with id #{id} from proxy, no host associated with report"
openscap_proxy_api.destroy_report(self, ForemanOpenscap::Helper::find_name_or_uuid_by_host(host))
Copy link
Member

Choose a reason for hiding this comment

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

we're no longer rescuing from errors on proxy? does that mean we'd render ugly error if proxy returns 500 e.g.? previously, the method in case of proxy error returned true (because of logger.debug), should we perhaps keep the rescue in this else branch?

Copy link
Member

Choose a reason for hiding this comment

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

ah scratch that, I see the rescue is handled in #destroy_report already and it returns false

@ares
Copy link
Member

ares commented Mar 2, 2018

turns out other error can occure because of taxnomies, the migration should be wrapped in User.as_anonymous_admin block, otherwise finding the openscap proxy can fail as User.current is nil during the migration

@xprazak2
Copy link
Contributor Author

xprazak2 commented Mar 5, 2018

I wrapped the migration in User.as_anonymous_admin block

@ares
Copy link
Member

ares commented Mar 9, 2018

Thanks @xprazak2, works as expected and fixes the issue, merging!. I'll cp to 0.7-stable.

@ares ares merged commit 7c4260d into theforeman:master Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants