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

[WIP] Add simple column defaults... still need to verify none had prior db … #721

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jrafanie
Copy link
Member

…defaults

@jrafanie jrafanie added the wip label Feb 29, 2024
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Feb 29, 2024
Depends on ManageIQ/manageiq-schema#721

Some of the remaining ones have custom defaults in STI or STI-like
subclasses, meaning we can't easily replace these with db defaults.

Defaults set in a subclass, not base class, in STI/STI like behavior:

  app/models/miq_provision_request.rb:  default_value_for :source_type,  "VmOrTemplate"
  app/models/service_template_provision_request.rb:  default_value_for :process,      false (overrides base class default)
  manageiq/providers/embedded_automation_manager/configuration_script_source.rb:  attribute :scm_type, :default => "git"
  manageiq/providers/embedded_automation_manager/configuration_script_source.rb:  attribute :scm_branch, :default => "master"

  manageiq/providers/cloud_manager/template.rb:  attribute :cloud, :default => true
  manageiq/providers/cloud_manager/vm.rb:  attribute :cloud, :default => true
  manageiq/providers/infra_manager/template.rb:  attribute :cloud, :default => false
  manageiq/providers/infra_manager/vm.rb:  attribute :cloud, :default => false
  manageiq/providers/physical_infra_manager/vm.rb:  attribute :cloud, :default => false

  app/models/service_retire_task.rb:  default_value_for :request_type, "service_retire"
  app/models/vm_migrate_task.rb:  default_value_for :request_type, "vm_migrate"
  app/models/vm_retire_task.rb:  default_value_for :request_type, "vm_retire"

  service_reconfigure_request.rb:  attribute :source_type, :default => SOURCE_CLASS_NAME
  service_template_provision_request.rb:  attribute :source_type, :default => SOURCE_CLASS_NAME
  vm_retire_request.rb:  attribute :source_type, :default => SOURCE_CLASS_NAME
  change_column_default :orchestration_stack_retire_tasks, :request_type,  :from => nil, :to => "orchestration_stack_retire"

Others, use block notation and isn't replaceable with database defaults.
We tried attribute defaults before but they trigger "changes" when the defaults
get set, which leads to unexpected changes in behavior.
@miq-bot
Copy link
Member

miq-bot commented Feb 29, 2024

Checked commit jrafanie@7748a2f with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 54 offenses detected

db/migrate/20240228190145_add_column_defaults.rb

@kbrock
Copy link
Member

kbrock commented Mar 1, 2024

  1. Think allow_null => false is good here.
  2. The allow null updates all the columns to the default value.
  3. But since the update is part of a schema change, it blocks the whole database. So people tend to manually update the database and then do the schema change (i.e.: disallow null) in a separate transaction. This makes the change very verbose. Maybe we want a helper?
{
  :table1 => {
    :column1 => default,
    :column2 => default
  }
}



def migrate_null_defaults(table_name, columns)
  # can we do a single query?
  columns.each do |column_name, default|
    change_column_default(table_name, column_name, default)
  end
  model.where(columns.transform_values {|n, v| v = nil}).update_all(columns)

  # can we do a single query?
  # verify this is that much different
  columns.each do |column_name, default|
    change_column_null(table_name, column_name, false, default)
  end
end

@miq-bot miq-bot added the stale label Jun 3, 2024
@miq-bot
Copy link
Member

miq-bot commented Jun 3, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants