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.

Sign up for GitHub

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

Jump to bottom

Remove DatabaseBackup #21415

Merged
merged 2 commits into from
Nov 4, 2021
Merged

Remove DatabaseBackup #21415

merged 2 commits into from
Nov 4, 2021

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Aug 31, 2021 •
edited
Loading

@miq-bot miq-bot added the wip label Aug 31, 2021
@NickLaMuro NickLaMuro mentioned this pull request Aug 31, 2021
Merged
@NickLaMuro NickLaMuro mentioned this pull request Aug 31, 2021
Merged
3 tasks
Fryguy
Fryguy reviewed Sep 1, 2021
View reviewed changes
app/models/miq_region.rb Outdated Show resolved Hide resolved
@jrafanie
Copy link
Member

jrafanie commented Sep 7, 2021

image

Hmm, I wonder what lj.out contained...

jrafanie
jrafanie reviewed Sep 7, 2021
View reviewed changes
spec/models/miq_schedule_filter_spec.rb
@@ -31,12 +31,6 @@
:sched_action => {:method => "vm_scan"},
:miq_search_id => @search.id
)

# DB Baskup Schedule
Copy link
Member

Choose a reason for hiding this comment

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

Baskup... 🤣

jrafanie
jrafanie reviewed Sep 7, 2021
View reviewed changes
app/models/miq_schedule.rb Outdated Show resolved Hide resolved
@NickLaMuro
Copy link
Member Author

NickLaMuro commented Sep 7, 2021 •
edited
Loading

Hmm, I wonder what lj.out contained...

@jrafanie it was from when we were debugging the locking issues with the UI:

lj.out contents... 
$ cat x.rb
trap(:INFO) {
  # Thread.list.each do |t|
  #   puts "#" * 90
  #   p t
  #   puts t.backtrace
  #   puts "#" * 90
  # end

  puts ActionDispatch::DebugLocks.new(nil).send(:render_details, nil).last.last
}
$ ruby -I. -rx bin/rails s

Fryguy
Fryguy reviewed Sep 7, 2021
View reviewed changes
db/fixtures/miq_event_definition_events.yml Outdated
Comment on lines 79 to 82
- name: evm_server_db_backup_low_space
description: Server Database Backup Insufficient Space
event_type: Default
set_type: evm_operations
Copy link
Member

Choose a reason for hiding this comment

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

Will this require a data migration to ensure that any existing event defintions aren't tied to any existing objects, or will seed just take care of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh... I don't know... I should probably look into that...

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how timelines or other graphs that show events over time would handle a reference to an event definition that no longer exists. Maybe it stored the definition information whatever it's displaying in the report result itself, similar to how we have a copy of the report definition in each result, in the situation where the report definition changed since it was included.

Copy link
Member

@Fryguy Fryguy Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I wasn't even thinking of reports / timelines - I was more thinking things like policies where the event is the driver for the policy. So we'd have to remove the event from the policy, and then I guess if there are no events left we delete the entire policy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it better that we just keep this then? I just removed it because it had db_backup in it, and I don't think anything else was going to be using it. But if we think it might have lingering uses, I don't mind just keeping it if it makes merging this simpler.

Copy link
Member

Choose a reason for hiding this comment

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

If we intend to backport, then we can leave it for this PR, but if not, let's drop it... keeping it around, but having it never fire is worse because it sets up a user expectation that is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I will remove this for now, but immediately open up a PR to fix this once it is merged so we can get most of this rolling, since I am unclear about what all is necessary, and I don't want this one thing holding things up.

Copy link
Member Author

@NickLaMuro NickLaMuro Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@Fryguy Here is the follow up so it is on our radar: #21430

Also, removed this section from this PR.

@NickLaMuro NickLaMuro force-pushed the delete_database_backup branch 2 times, most recently from 99001f1 to 5c4d6c6 Compare September 14, 2021 14:08
@NickLaMuro NickLaMuro mentioned this pull request Sep 14, 2021
Closed
@kbrock
Copy link
Member

kbrock commented Sep 28, 2021

not sure if we need a rebase or just to kick off the build again. I kicked one of them and will see if that fixes the first travis build

@NickLaMuro NickLaMuro mentioned this pull request Nov 2, 2021
Closed
15 tasks
@miq-bot
Copy link
Member

miq-bot commented Nov 3, 2021

Checked commits NickLaMuro/manageiq@5dc71b5~...0e59ada with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
5 files checked, 0 offenses detected
Everything looks fine. 👍

@NickLaMuro NickLaMuro changed the title [WIP] Remove DatabaseBackup Remove DatabaseBackup Nov 3, 2021
@miq-bot miq-bot removed the wip label Nov 3, 2021
@Fryguy Fryguy merged commit a062596 into ManageIQ:master Nov 4, 2021
@jrafanie jrafanie mentioned this pull request Jun 15, 2022
Closed
@Fryguy Fryguy mentioned this pull request Aug 1, 2023
Open
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare Awaiting requested review from agrare agrare is a code owner

@kbrock kbrock Awaiting requested review from kbrock kbrock is a code owner

@bdunne bdunne Awaiting requested review from bdunne

@Fryguy Fryguy Awaiting requested review from Fryguy Fryguy is a code owner

@jrafanie jrafanie Awaiting requested review from jrafanie jrafanie is a code owner

Assignees

@Fryguy Fryguy

Labels
technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@NickLaMuro @jrafanie @kbrock @miq-bot @Fryguy @chessbyte