[WIP] Refactor run_single_worker find or create record #21436

agrare · 2021-09-15T18:18:52Z

Everytime I read this method my head hurts, time to fix it

agrare · 2021-09-15T18:20:14Z

lib/workers/bin/run_single_worker.rb

@@ -107,20 +107,14 @@ def all_role_names
  end

  update_options = create_options.dup


NOTE I'm not sure why we're dup'ing create_options here anymore, we used to add in pid / system_uid to update_options but not after #21068 as far as I can tell

Yeah that's good point - there's no need to dup it or even have the var at all other than readability (i.e. we care about updates here vs creates)

Yeah, 😕 ... I'm not sure why that remained. Good eye.

agrare · 2021-09-15T18:21:25Z

lib/workers/bin/run_single_worker.rb

-           else
-             worker_class.create_worker_record(create_options)
-           end
+  find_options   = options.slice(:guid, :system_uid)


NOTE need to check if either of these ever get passed in as nil in which case we would have to drop those

Hmm, I'm concerned about this part. I'd rather get the appliance side to use system_uid and drop guid. Podified should always have a system_uid, I think. I was reluctant to change the appliance side at that time.

Yeah that's a great point, we can update to pass in the systemd unit name and drop GUID entirely

We are now setting the system_uid for systemd workers as well as of #22915

agrare · 2021-09-15T18:23:09Z

lib/workers/bin/run_single_worker.rb

+  # Podified needs to create on the first run_single_worker and update after since
+  # the GUID can't be predetermined for each replica
+  worker   = worker_class.find_by(find_options).tap { |w| w.update(update_options) } if find_options.present?
+  worker ||= worker_class.create_worker_record(create_options)


NOTE could we extract MiqWorker.init_worker_object params and do a simple find_or_update ?

Fryguy · 2021-09-15T19:27:13Z

I don't think this is a clean refactoring, particularly if both guid and system-uid are passed (not sure that's possible). Even so, the first conditional is find_by!(:guid) (note the !) and the second is find_by(:system_uid) (note the non-!), but this changes it to find_by(:guid AND :system_uid).

agrare · 2021-09-15T19:31:07Z

Oh yeah this definitely changes the behavior of if a GUID is passed and not found to reduce the complexity (agree with the [comment] TODO: This is really inconsistent and confusing. Why can't GUID follow the same rules?)

Maybe refactor is not accurate, mor simplify/cleanup

miq-bot · 2023-02-27T00:17:41Z

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-05-29T00:01:18Z

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

Feel free to reopen this pull request if these changes are still valid.

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

kbrock · 2023-07-13T01:52:56Z

is this still valid?

miq-bot · 2023-10-16T00:00:45Z

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-10-17T20:34:33Z

Is there a reason this is WIP?
LGTM

agrare · 2023-10-17T20:40:54Z

Possibly due to #21436 (comment) ? I need to revisit this as it does clean this code up quite a bit.

miq-bot · 2024-01-22T00:00:23Z

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-04-29T00:00:38Z

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

kbrock · 2024-04-29T17:09:57Z

lib/workers/bin/run_single_worker.rb

-           else
-             worker_class.create_worker_record(create_options)
-           end
+  find_options = options.slice(:guid, :system_uid)


Suggested change

find_options = options.slice(:guid, :system_uid)

find_options = options.slice(:guid, :system_uid).compact

This will clear out nil values.

I'm not sure that worrying about the user specifying 2 ids is warranted.

Can we merge after this change?

miq-bot · 2024-04-30T12:20:38Z

Checked commits agrare/manageiq@5ee7882~...c8f9602 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍪

miq-bot · 2024-07-25T19:05:27Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2024-11-01T01:36:36Z

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 · 2025-02-03T00:00:33Z

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

agrare requested a review from Fryguy as a code owner September 15, 2021 18:18

agrare added core refactoring labels Sep 15, 2021

agrare commented Sep 15, 2021

View reviewed changes

agrare changed the title ~~Refactor run_single_worker find or create record~~ [WIP] Refactor run_single_worker find or create record Sep 15, 2021

agrare commented Sep 15, 2021

View reviewed changes

miq-bot added the wip label Sep 15, 2021

Fryguy assigned jrafanie Sep 15, 2021

miq-bot added the stale label Feb 27, 2023

miq-bot closed this May 29, 2023

kbrock removed the stale label Jul 13, 2023

kbrock reopened this Jul 13, 2023

miq-bot added the stale label Oct 16, 2023

kbrock removed the stale label Oct 17, 2023

miq-bot added the stale label Jan 22, 2024

kbrock reviewed Apr 29, 2024

View reviewed changes

agrare added 3 commits April 30, 2024 08:16

Refactor run_single_worker find or create record

5ee7882

Unify update_options/create_options

f979ab1

Always use system_uid for finding worker record

c8f9602

agrare force-pushed the refactor_run_single_worker_find_record branch from e08b0fc to c8f9602 Compare April 30, 2024 12:18

miq-bot added the unmergeable label Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Refactor run_single_worker find or create record #21436

[WIP] Refactor run_single_worker find or create record #21436

agrare commented Sep 15, 2021

agrare Sep 15, 2021

Fryguy Sep 15, 2021 •

edited

Loading

jrafanie Sep 15, 2021

agrare Sep 15, 2021

jrafanie Sep 15, 2021

agrare Sep 15, 2021

agrare Apr 29, 2024

agrare Sep 15, 2021

Fryguy commented Sep 15, 2021 •

edited

Loading

agrare commented Sep 15, 2021 •

edited

Loading

miq-bot commented Feb 27, 2023

miq-bot commented May 29, 2023

kbrock commented Jul 13, 2023

miq-bot commented Oct 16, 2023

kbrock commented Oct 17, 2023

agrare commented Oct 17, 2023

miq-bot commented Jan 22, 2024

miq-bot commented Apr 29, 2024

kbrock Apr 29, 2024

miq-bot commented Apr 30, 2024

miq-bot commented Jul 25, 2024

miq-bot commented Nov 1, 2024

miq-bot commented Feb 3, 2025

		@@ -107,20 +107,14 @@ def all_role_names
		end

		update_options = create_options.dup

	find_options = options.slice(:guid, :system_uid)
	find_options = options.slice(:guid, :system_uid).compact

[WIP] Refactor run_single_worker find or create record #21436

Are you sure you want to change the base?

[WIP] Refactor run_single_worker find or create record #21436

Conversation

agrare commented Sep 15, 2021

Choose a reason for hiding this comment

Fryguy Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Sep 15, 2021 • edited Loading

agrare commented Sep 15, 2021 • edited Loading

miq-bot commented Feb 27, 2023

miq-bot commented May 29, 2023

kbrock commented Jul 13, 2023

miq-bot commented Oct 16, 2023

kbrock commented Oct 17, 2023

agrare commented Oct 17, 2023

miq-bot commented Jan 22, 2024

miq-bot commented Apr 29, 2024

Choose a reason for hiding this comment

miq-bot commented Apr 30, 2024

miq-bot commented Jul 25, 2024

miq-bot commented Nov 1, 2024

miq-bot commented Feb 3, 2025

Fryguy Sep 15, 2021 •

edited

Loading

Fryguy commented Sep 15, 2021 •

edited

Loading

agrare commented Sep 15, 2021 •

edited

Loading