Refactor MiqProvisionWorkflow#initialize method and subclasses #21014

chessbyte · 2021-02-05T15:13:05Z

No description provided.

Fryguy · 2021-02-05T15:19:43Z

Should these new methods be private methods?

chessbyte · 2021-02-05T15:19:58Z

@lfu when you get a chance, if you can review and validate in a real provision workflow (I don't have environment to try it), that would be great. This is NOT URGENT and very low priority.

chessbyte · 2021-02-05T15:22:18Z

@lfu I re-ordered the initialize method here because it seemed that the dialog initialization and configuration has nothing to do with the password_helper and setting instance variables. Let me know if my assumptions are wrong.

app/models/miq_request_workflow.rb

Fryguy · 2021-02-05T15:23:05Z

app/models/miq_request_workflow.rb

    end
+    normalize_numeric_fields unless @dialogs.nil?


I'm curious why this was moved from the if @dialogs.nil? branch

that is the way some of the subclasses had it. so I commonized the code.

Fryguy · 2021-02-05T15:24:51Z

app/models/miq_provision_virt_workflow.rb

+  def initialize(values, requester, options = {})
+    super
+
+    load_source_object if initial_pass?(values, options)


Does this have to happen before the dialogs are loaded? (i.e. in between initialize_dialogs and configure_dialogs?)

good question - maybe @lfu knows.

See #21014 (comment)

app/models/miq_provision_orch_workflow.rb

miq-bot · 2023-02-27T00:18:49Z

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)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

kbrock · 2023-05-03T15:22:33Z

This removes a bunch of copy paste code.

Do we want to pick this up, or let stale make it disappear?

Fryguy · 2023-05-16T19:41:30Z

I'm surprised this PR is still mergeable - would like @agrare to review and then either accept or close.

miq-bot · 2023-08-21T00:00:04Z

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)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot · 2023-11-27T00:00:15Z

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).

miq-bot · 2024-03-04T00:00:07Z

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).

miq-bot · 2024-06-10T00:00:11Z

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).

miq-bot · 2024-09-16T00:00:18Z

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).

…cal to MiqProvisionVirtWorkflow#initialize

…rkflow#initialize

…ject_class

…s and configure_dialogs

… spec failures

miq-bot · 2024-09-17T18:50:45Z

Checked commits chessbyte/manageiq@55f03f0~...c7243c9 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 5 offenses detected

app/models/miq_provision_virt_workflow.rb

⚠️ - Line 1117, Col 3 - Lint/IneffectiveAccessModifier - private (on line 965) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.

app/models/miq_request_workflow.rb

⚠️ - Line 1524, Col 26 - Lint/UnusedMethodArgument - Unused method argument - values. If it's necessary, use _ or _values as an argument name to indicate that it won't be used. If it's unnecessary, remove it.
⚠️ - Line 1535, Col 25 - Lint/UnusedMethodArgument - Unused method argument - values. If it's necessary, use _ or _values as an argument name to indicate that it won't be used. If it's unnecessary, remove it. You can also write as configure_dialogs(*) if you want the method to accept any arguments but don't care about them.
⚠️ - Line 1535, Col 33 - Lint/UnusedMethodArgument - Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used. If it's unnecessary, remove it. You can also write as configure_dialogs(*) if you want the method to accept any arguments but don't care about them.
❗ - Line 1530, Col 34 - Style/IfInsideElse - Convert if nested inside else to elsif.

kbrock · 2024-09-17T18:51:45Z

update:

rebased to fix conflict

Can fix cops if we deem we want to still work on this

@Fryguy @agrare do we still want this, or just close?

agrare

LGTM, a couple of the options[:skip_dialog_load] == true look like they could drop the == true but that is just moving existing code around.

chessbyte added the refactoring label Feb 5, 2021

Fryguy reviewed Feb 5, 2021

View reviewed changes

app/models/miq_request_workflow.rb Outdated Show resolved Hide resolved

Fryguy reviewed Feb 5, 2021

View reviewed changes

app/models/miq_provision_orch_workflow.rb Outdated Show resolved Hide resolved

chessbyte force-pushed the refactor_miq_provision_worflow_initialize_method_and_subclasses branch from 77b9dea to 518f6d9 Compare February 5, 2021 15:38

gtanzillo assigned Fryguy Feb 11, 2021

miq-bot added the stale label Feb 27, 2023

Fryguy assigned agrare and unassigned Fryguy May 16, 2023

Fryguy removed the stale label May 16, 2023

miq-bot added the stale label Aug 21, 2023

miq-bot added the unmergeable label Mar 7, 2024

chessbyte added 6 commits September 17, 2024 14:43

Extract initial_pass? method

55f03f0

Extract load_source_object method

f5a4f36

Extract initialize_dialogs method

bfcae12

Extract configure_dialogs method

316b6f7

Remove MiqProvisionOrchWorkflow#initialize method as it is not identi…

f3bd847

…cal to MiqProvisionVirtWorkflow#initialize

Refactor MiqProvisionVirtWorkflow#initialize to leverage MiqRequestWo…

4597573

…rkflow#initialize

chessbyte added 4 commits September 17, 2024 14:43

DRYed up load_source_object method by creating class method source_ob…

694eeff

…ject_class

Move extracted methods to private area of the code

3e3e977

Check options[:skip_dialog_load] outside of methods initialize_dialog…

5a62aae

…s and configure_dialogs

Removed private_class_method on source_object_class as it was causing…

c7243c9

… spec failures

kbrock force-pushed the refactor_miq_provision_worflow_initialize_method_and_subclasses branch from 4094852 to c7243c9 Compare September 17, 2024 18:47

kbrock requested review from agrare and kbrock as code owners September 17, 2024 18:47

agrare added lifecycle/provisioning and removed stale unmergeable labels Sep 18, 2024

agrare approved these changes Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MiqProvisionWorkflow#initialize method and subclasses #21014

Refactor MiqProvisionWorkflow#initialize method and subclasses #21014

chessbyte commented Feb 5, 2021

Fryguy commented Feb 5, 2021

chessbyte commented Feb 5, 2021

chessbyte commented Feb 5, 2021

Fryguy Feb 5, 2021 •

edited

Loading

chessbyte Feb 5, 2021

Fryguy Feb 5, 2021

chessbyte Feb 5, 2021

chessbyte Feb 5, 2021

miq-bot commented Feb 27, 2023

kbrock commented May 3, 2023

Fryguy commented May 16, 2023

miq-bot commented Aug 21, 2023

miq-bot commented Nov 27, 2023

miq-bot commented Mar 4, 2024

miq-bot commented Jun 10, 2024

miq-bot commented Sep 16, 2024

miq-bot commented Sep 17, 2024

kbrock commented Sep 17, 2024

agrare left a comment

Refactor MiqProvisionWorkflow#initialize method and subclasses #21014

Are you sure you want to change the base?

Refactor MiqProvisionWorkflow#initialize method and subclasses #21014

Conversation

chessbyte commented Feb 5, 2021

Fryguy commented Feb 5, 2021

chessbyte commented Feb 5, 2021

chessbyte commented Feb 5, 2021

Fryguy Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

chessbyte Feb 5, 2021

Choose a reason for hiding this comment

Fryguy Feb 5, 2021

Choose a reason for hiding this comment

chessbyte Feb 5, 2021

Choose a reason for hiding this comment

chessbyte Feb 5, 2021

Choose a reason for hiding this comment

miq-bot commented Feb 27, 2023

kbrock commented May 3, 2023

Fryguy commented May 16, 2023

miq-bot commented Aug 21, 2023

miq-bot commented Nov 27, 2023

miq-bot commented Mar 4, 2024

miq-bot commented Jun 10, 2024

miq-bot commented Sep 16, 2024

miq-bot commented Sep 17, 2024

kbrock commented Sep 17, 2024

agrare left a comment

Choose a reason for hiding this comment

Fryguy Feb 5, 2021 •

edited

Loading