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

Add a default params_to_attributes for EmbeddedAutomationManager Authentications #22570

Merged
merged 4 commits into from
Jun 22, 2023
Merged

Add a default params_to_attributes for EmbeddedAutomationManager Authentications #22570

merged 4 commits into from
Jun 22, 2023

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 16, 2023 •
edited by kbrock
Loading

Use API_ATTRIBUTES to filter allowed parameters for embedded automation manager authentications

followup:

@agrare agrare force-pushed the add_default_params_to_attributes branch from ea67932 to ae7c19c Compare June 16, 2023 23:57
@agrare
Copy link
Member Author

agrare commented Jun 16, 2023

@miq-bot cross-repo-tests manageiq-providers-awx, manageiq-providers-ansible_tower, manageiq-api, manageiq-ui-classic

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jun 16, 2023
@agrare @miq-bot
Running tests for agrare
51ac132 
From Pull Request:  ManageIQ/manageiq#22570
@miq-bot miq-bot mentioned this pull request Jun 16, 2023
Closed
@agrare agrare force-pushed the add_default_params_to_attributes branch from ae7c19c to 4696948 Compare June 17, 2023 18:19
@agrare
Copy link
Member Author

agrare commented Jun 22, 2023 •
edited
Loading

@miq-bot cross-repo-tests manageiq-providers-awx, manageiq-providers-ansible_tower, manageiq-api, manageiq-ui-classic, manageiq-providers-workflows

@miq-bot miq-bot mentioned this pull request Jun 22, 2023
Closed
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jun 22, 2023
@agrare @miq-bot
Running tests for agrare
9f51d29 
From Pull Request:  ManageIQ/manageiq#22570
@agrare agrare changed the title [WIP] Add a default params_to_attributes for EmbeddedAutomationManager Authentications Add a default params_to_attributes for EmbeddedAutomationManager Authentications Jun 22, 2023
@agrare agrare force-pushed the add_default_params_to_attributes branch from 270921d to 20eedfc Compare June 22, 2023 13:47
@miq-bot miq-bot removed the wip label Jun 22, 2023
@miq-bot
Copy link
Member

miq-bot commented Jun 22, 2023

Checked commits agrare/manageiq@4696948~...20eedfc with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
11 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare requested review from kbrock and Fryguy June 22, 2023 15:15
@agrare
Copy link
Member Author

agrare commented Jun 22, 2023

Okay this is ready for review

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jun 22, 2023
@agrare @miq-bot
Running tests for agrare
dba5bca 
From Pull Request:  ManageIQ/manageiq#22570
@miq-bot miq-bot mentioned this pull request Jun 22, 2023
Closed
agrare added a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jun 22, 2023
@agrare
Running tests for agrare
d14b1a7 
From Pull Request:  ManageIQ/manageiq#22570
@agrare
Copy link
Member Author

agrare commented Jun 22, 2023

Cross-repo is green ManageIQ/manageiq-cross_repo-tests#805

kbrock
kbrock reviewed Jun 22, 2023
View reviewed changes
app/models/manageiq/providers/embedded_automation_manager/authentication.rb
def self.params_to_attributes(_params)
raise NotImplementedError, "must be implemented in a subclass"
def self.params_to_attributes(params)
allowed_params = self::API_ATTRIBUTES.pluck(:id) + %w[name type options]
Copy link
Member

Choose a reason for hiding this comment

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

did you want to put name, type, options into COMMON_ATTRIBUTES?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because API_ATTRIBUTES is used by ui-classic to build dynamic dialogs (https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/ansible_credential_helper/textual_summary.rb#L15) so if we add them there they will show up on the UI which we don't want.

@kbrock kbrock assigned kbrock and unassigned Fryguy Jun 22, 2023
@kbrock kbrock added the bug label Jun 22, 2023
@kbrock kbrock merged commit a928eaa into ManageIQ:master Jun 22, 2023
9 checks passed
@agrare agrare deleted the add_default_params_to_attributes branch June 22, 2023 17:55
@agrare agrare mentioned this pull request Jun 22, 2023
Merged
@kbrock kbrock added enhancement and removed bug labels Jun 22, 2023
@agrare agrare mentioned this pull request Jun 22, 2023
Merged
@kbrock kbrock mentioned this pull request Jun 22, 2023
Closed
@Fryguy
Copy link
Member

Fryguy commented Jun 28, 2023

Backported to petrosian in commit 51a1dca.

commit 51a1dca7a6349fc07b5f4f1c064aab4766643003
Author: Keenan Brock <[email protected]>
Date:   Thu Jun 22 13:52:09 2023 -0400

    Merge pull request #22570 from agrare/add_default_params_to_attributes
    
    Add a default params_to_attributes for EmbeddedAutomationManager Authentications
    
    (cherry picked from commit a928eaafa2e85d114d36a5f65ea9b847a0ae0159)

Fryguy pushed a commit that referenced this pull request Jun 28, 2023
@kbrock @Fryguy
Merge pull request #22570 from agrare/add_default_params_to_attributes
51a1dca 
Add a default params_to_attributes for EmbeddedAutomationManager Authentications

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

@kbrock kbrock kbrock left review comments

@Fryguy Fryguy Awaiting requested review from Fryguy

Assignees

@kbrock kbrock

Labels
core/embedded ansible enhancement petrosian/backported
Projects
None yet
Milestone
No milestone
Development

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