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

Introduce API endpoints for creating/editing Automate Domains #647

Closed
wants to merge 1 commit into from

Conversation

skateman
Copy link
Member

@skateman skateman commented Aug 9, 2019

Required in ManageIQ/manageiq-ui-classic#5971

@miq-bot add_label hammer/no, ivanchuk/no, changelog/yes, enhancement
@miq-bot add_reviewer @lpichler
@miq-bot add_reviewer @abellotti
@miq-bot add_reviewer @d-m-u

@skateman skateman force-pushed the ae_domain-create-edit branch from 2a3b474 to df56179 Compare August 14, 2019 10:53
@skateman skateman changed the title [WIP] Introduce API endpoints for creating/editing Automate Domains Introduce API endpoints for creating/editing Automate Domains Aug 14, 2019
@miq-bot miq-bot removed the wip label Aug 14, 2019
@miq-bot
Copy link
Member

miq-bot commented Aug 14, 2019

Checked commit skateman@df56179 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@abellotti
Copy link
Member

/cc @mkanoor

for creates, any additional checks needed or are we ok from the model side of things ?


expect(domain.name).not_to eq('foo')

put(api_automate_domain_url(nil, domain), :params => {'name' => 'foo'})
Copy link
Contributor

Choose a reason for hiding this comment

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

@skateman we might need work in the backend model to limit what attributes can be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mkanoor okay, can you please be more specific?

Copy link
Member

Choose a reason for hiding this comment

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

@mkanoor : ping?

We would like to move forward with this as it blocks UI work in progress.

Or shall we refer to someone from the core team to work on this on the model side?

@martinpovolny
Copy link
Member

@skateman : I would suggest using the available helper methods for assertions.
Also I'd add a test to check that w/o the privileges one cannot edit/add.
Thx!

@mkanoor
Copy link
Contributor

mkanoor commented Oct 16, 2019

@skateman @martinpovolny we cannot have an API that allows you to change all the attributes in an MiqAeDomain object. This will allow users to unlock ManageIQ domain which is a system protected domain.
I had discussed this with @tinaafitz we can modify the import process to have an optional parameter that can enable a domain, During import itself we would allow you to enable the domain with the caveat that it is not recommended to enable Domains on the fly without a change control process in place. The default option for import would be to have the domain be disabled. But if the user wants to enable during import they would be able to do that.

This API seems like it is a 2 step process to overcome a Change Control process that we wanted administrators to enforce.

  1. Run Import
  2. Then call this API to enable the Domain

We can do this in a single step

  1. Run import and enable the domain.

@skateman
Copy link
Member Author

@mkanoor I don't really understand what the import has to do with the API endpoint. I just want to use this in the UI forms that create (and edit some parts of) automate domains. In the current angular implementation, which is not API driven we have this logic:

    name_readonly = @ae_ns.domain? && !@ae_ns.editable_property?(:name)
    description_readonly = @ae_ns.domain? && !@ae_ns.editable_property?(:description)

I have no idea how to expose properties through the API 😕

@mkanoor
Copy link
Contributor

mkanoor commented Oct 17, 2019

@skateman There was another thread where someone was trying to edit the Automate Domain for a similar purpose for Git Imports via REST API. We can have a generic edit Automate Domain for updating attributes, but we would need to guard and honor some of the checks that are in the miq_ae_domain.
Before you update a property you would have to check if the property is editable
https://github.com/ManageIQ/manageiq-automation_engine/blob/9ab733b5fce3e6c1361482702558d355727ad5a3/app/models/miq_ae_domain.rb#L88
for all the attributes coming in from the caller.

If not all properties are editable we should return a 400/403?

I am not sure if that code is here or is this just adding.
There are also Tenancy concerns.

It would be better if Automate overrode the update_attributes for MiqAeDomain and you would call update_attributes form your API code.

@tinaafitz can we have someone from Automate Team work on this

@martinpovolny
Copy link
Member

Thanks @mkanoor, for all the information. Please, discuss with @tinaafitz as you suggested so that we can move forward with the work on the API side.

@skateman: I think that you can move ahead and implement it the way @mkanoor wrote and once there's that method implemented by automate, just use it.

Do you agree, guys?

@tinaafitz
Copy link
Member

Hi @skateman @martinpovolny, yes, I agree that we should move forward with the implementation that @mkanoor suggested. I spoke to @lfu about it at a high level yesterday, and will discuss it with her in more detail.

@skateman
Copy link
Member Author

skateman commented Oct 24, 2019

As my comment here I think the writability should be sorted out on the model level. However, we need an endpoint to ask the API if that given entity is writable or not and with what fields.

My idea would be an OPTIONS request that would respond what fields are writable, but not sure about how can we do that using editable_property? on a given record. cc @martinpovolny

@tinaafitz
Copy link
Member

@skateman @martinpovolny I discussed this with @mkanoor and @abellotti, and created a document on what Domain UI operations are available today, and what they update. I think we'll need multiple API calls to handle the different update scenarios.

https://docs.google.com/document/d/1lz5T-C1W1k59wJu4sonqpU4B80VROzzaMM7_qiLl2Ic/edit?usp=sharing

@martinpovolny
Copy link
Member

ping @Fryguy

@chessbyte
Copy link
Member

@Fryguy sounds like we need an architectural plan on this as this PR seems stuck

@skateman
Copy link
Member Author

We actually discussed this with @Fryguy during my last visits in Mahwah, there were some plans, but really it hangs on his side 😕

@Fryguy
Copy link
Member

Fryguy commented Apr 7, 2020

Yes, this is held up on me at the moment

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2022

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

2 similar comments
@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2022

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

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2022

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

@miq-bot
Copy link
Member

miq-bot commented Jan 25, 2022

@skateman Cannot add the following reviewer because they are not recognized: lpichler

@miq-bot
Copy link
Member

miq-bot commented Jan 25, 2022

@skateman Cannot add the following reviewer because they are not recognized: abellotti

@miq-bot
Copy link
Member

miq-bot commented Jan 25, 2022

@skateman Cannot add the following reviewer because they are not recognized: d-m-u

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.

8 participants