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

Delegate Provisioning refresh to Configuration #113

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Delegate Provisioning refresh to Configuration #113

merged 1 commit into from
Mar 22, 2023

Conversation

agrare
Copy link
Member

@agrare agrare commented Mar 20, 2023 •
edited
Loading

@agrare agrare requested a review from Fryguy as a code owner March 20, 2023 14:34
@agrare agrare mentioned this pull request Mar 20, 2023
Merged
@miq-bot
Copy link
Member

miq-bot commented Mar 20, 2023

Checked commit agrare@35ec818 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@Fryguy
Copy link
Member

Fryguy commented Mar 20, 2023

Should this be petrosian/yes?

@Fryguy Fryguy added the bug label Mar 20, 2023
@Fryguy Fryguy self-assigned this Mar 20, 2023
Fryguy
Fryguy reviewed Mar 20, 2023
View reviewed changes
config/settings.yml
@@ -6,8 +6,6 @@
:ems_refresh:
:foreman_configuration:
:refresh_interval: 15.minutes
:foreman_provisioning:
:refresh_interval: 1.hour
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a data migration to remove any possibly overriden settings?

Copy link
Member

Choose a reason for hiding this comment

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

good question, I'm not sure what's looking for these possible values and if they'll gracefully handle old settings ... or we need to drop any changes from the settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure we can do that

Copy link
Member Author

Choose a reason for hiding this comment

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

ManageIQ/manageiq-schema#685

Fryguy
Fryguy approved these changes Mar 20, 2023
View reviewed changes
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

@jrafanie Would also like your review since this is reflected in ManageIQ/manageiq#22419

jrafanie
jrafanie reviewed Mar 21, 2023
View reviewed changes
app/models/manageiq/providers/foreman/configuration_manager/refresh_worker.rb
def queue_name_for_ems(ems)
return ems unless ems.kind_of?(ExtManagementSystem)

ems.provider.managers.collect(&:queue_name).sort
Copy link
Member

Choose a reason for hiding this comment

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

[] => 🗑️ 🔥 🚽

😄

@jrafanie
Copy link
Member

LGTM, other than the question about needing a migration.

@agrare agrare mentioned this pull request Mar 22, 2023
Merged
1 task
@Fryguy Fryguy merged commit 91fb022 into ManageIQ:master Mar 22, 2023
@agrare agrare deleted the delegate_refresh_to_configuration_manager branch March 22, 2023 17:41
@Fryguy
Copy link
Member

Fryguy commented Mar 30, 2023

Backported to petrosian in commit c875ee1.

commit c875ee12066fbf2ca74671e75f79a2d187240a2e
Author: Jason Frey <fryguy9@gmail.com>
Date:   Wed Mar 22 13:40:15 2023 -0400

    Merge pull request #113 from agrare/delegate_refresh_to_configuration_manager
    
    Delegate Provisioning refresh to Configuration
    
    (cherry picked from commit 91fb022320f12eb6289df6e4aa050251992fa1d3)

Fryguy added a commit that referenced this pull request Mar 30, 2023
@Fryguy
Merge pull request #113 from agrare/delegate_refresh_to_configuration…
c875ee1 
…_manager

Delegate Provisioning refresh to Configuration

(cherry picked from commit 91fb022)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrafanie jrafanie jrafanie left review comments

@Fryguy Fryguy Fryguy approved these changes

Assignees

@Fryguy Fryguy

Labels
bug petrosian/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agrare @miq-bot @Fryguy @jrafanie