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

Pluggable Server Roles #22918

Merged
merged 5 commits into from
Feb 29, 2024
Merged

Pluggable Server Roles #22918

merged 5 commits into from
Feb 29, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 28, 2024

Allow plugins to bring their own server_roles by adding a content/server_roles.csv file, for example if we extracted Embedded Ansible into a plugin we could move the embedded_ansible role to manageiq-providers-embedded_ansible/content/server_roles.csv:

name,description,max_concurrent,external_failover,role_scope
embedded_ansible,Embedded Ansible,0,false,region

#19440

@@ -55,7 +55,7 @@
web_services,Web Services,0,false,region
CSV

allow(File).to receive(:open).and_return(StringIO.new(@csv))
allow(File).to receive(:open).with(ServerRole.fixture_path, "r", any_args).and_return(StringIO.new(@csv))
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't actually necessary, I just don't think mocking File.open without any args is error prone

@agrare agrare force-pushed the pluggable_server_roles branch 2 times, most recently from 794120d to 04ee52f Compare February 28, 2024 17:12
@agrare agrare mentioned this pull request Feb 28, 2024
48 tasks
@@ -55,7 +55,11 @@
web_services,Web Services,0,false,region
CSV

allow(File).to receive(:open).and_return(StringIO.new(@csv))
allow(File).to receive(:exist?).and_call_original
allow(File).to receive(:exist?).with(a_string_matching(/content\/server_roles.csv/)).and_return(false)
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to switch to using File.exist?(file.to_s) to make it simpler to stub there here, open to other suggestions on mocking Pathname#exist? though

Copy link
Member

Choose a reason for hiding this comment

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

I have to find it, but I've done similar tests for other plugins methods where I want to ensure it will pick up all the files from plugins. See vmdb/plugins_spec.rb. Then, I know I've created seed methods that show handling plugin based content, but I gotta find it.

@agrare agrare force-pushed the pluggable_server_roles branch from 21ea5ce to e8605bb Compare February 28, 2024 20:33
Comment on lines 108 to 111
@server_role_paths ||= filter_map do |engine|
file = engine.root.join("content/server_roles.csv")
file if File.exist?(file.to_s)
end
Copy link
Member

@kbrock kbrock Feb 28, 2024

Choose a reason for hiding this comment

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

Does this need a compact?

I see the if and that says to me that a nil could be returned.
So I'm guessing a nil could come out of filter_map result?

Copy link
Member Author

Choose a reason for hiding this comment

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

Filter_map drops the nils for you, this was a rubocop recommendation (replacing .map {}.compact with .filter_map {})

@Fryguy
Copy link
Member

Fryguy commented Feb 28, 2024

I suggest making this config/server_roles.csv instead. I think of content as more for things like out of the box content for ansible and workflows. This is more of basic configuration.

@agrare agrare force-pushed the pluggable_server_roles branch from e8605bb to 67c3971 Compare February 29, 2024 15:41
@agrare
Copy link
Member Author

agrare commented Feb 29, 2024

Changed to use config/server_roles.csv
Changed specs to stub the Vmdb::Plugins call rather than mess with trying to stub file exist?

@miq-bot
Copy link
Member

miq-bot commented Feb 29, 2024

Checked commits agrare/manageiq@fdc4907~...67c3971 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy Fryguy merged commit 95269ac into ManageIQ:master Feb 29, 2024
8 checks passed
@agrare agrare deleted the pluggable_server_roles branch February 29, 2024 19:17
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.

4 participants