AAP-32688: Support deletion of WCA API_KEY from Admin Portal (Backend)

[mabulgu]

Jira Issue: https://issues.redhat.com/browse/AAP-32688

Description

See issue: https://issues.redhat.com/browse/AAP-32688

Testing

• Follow the basic test steps that are in AAP-30986: Support deletion of WCA API_KEY from Admin Portal (Frontend) #1333 (but not with mocked backend but with a running backend on your local.)
• Run the ai.api.wca.tests.

Scenarios tested

• Delete api key and model id
• Delete api key without model id
• Try to delete api key that doesnt exist
• Fail deleting model id and see api key is not deleted afterward.

Production deployment

• [] This code change is ready for production on its own
• This code change requires the following considerations before going to production: End to end tests should be added with a seperate PR on tests repo (Thanks @jameswnl for the reminder!)

[manstis]

I know this is a draft; but thought I'd take a quick "Friday afternoon" look.

[manstis]

LGTM 👍

I know @jameswnl is concerned about the lack of an atomic transaction around deletion of the two secrets in AWS Secret Manager. I had a read and it does not seem possible to ensure a single transaction around their deletion.

I'm not sure what we can do about this.. other than moving the onus onto Users and having separate support for deleting the API Key and deleting the Model ID. Deleting the API Key but not* the Model ID leaves the system in an inconsistent state.

@jameswnl Thoughts?

[mabulgu]

it does not seem possible to ensure a single transaction around their deletion.

@manstis yep. it's a concern. I actually thought about this but somehow missed to call again among all other things. What I would suggest is: we can try deleting model id first, then if it is successfull we can try deleting the key. If the model deletion fails, then we dont delete the key, but if we can delete the model but not the key, at least we will not end up with sth inconsistent but sth possible (an org can have a key but not a model id set). wdyt?

[manstis]

@mabulgu

@manstis yep. it's a concern. I actually thought about this but somehow missed to call again among all other things. What I would suggest is: we can try deleting model id first, then if it is successfull we can try deleting the key. If the model deletion fails, then we don't delete the key, but if we can delete the model but not the key, at least we will not end up with sth inconsistent but sth possible (an org can have a key but not a model id set). wdyt?

That seems a reasonable approach.

You might need to consider how we can propagate a scenario when deletion of the Model ID was successful but deletion of the API Key resulted in an error. We should be able to send some form of response/error back that we can use to show an information message to Users... it's an edge case; but might be worth considering.

[jameswnl]

@manstis my suggestion to decouple the 2 deletions is meant to simply our logic (handling partial success situations etc). since you guys already did the handwork to handle them. I am fine :)

[mabulgu]

@manstis can you recheck when you have time pls? what are the changes:

• Some test improvements as we changed the deletion order and added signals
• Signals for deletions
• Modal confirmation text change

[mabulgu]

@manstis @jameswnl once this goes to stage I will add a test here to cover deletion as well: https://github.com/ansible/ansible-wisdom-testing/blob/main/tests/ui/test_admin_ui.py

Thanks

[goneri]

My understanding was that if the api_key is missing, we can safely ignore the presence model_id. This because without the api_key, it won't be used and if the user set a new api_key, the model_id will also be overwritten.

I don't think we need to do an atomic change. Did I miss something?

[jameswnl]

I don't think we need to do an atomic change. Did I miss something?

There's a misunderstanding about the need for atomic change (I probably unintentionally planted this idea into @manstis ' head).

My suggestion is really about this

@goneri thanks for the review. We reverted the logic where your comments were about. so hope you dont mind if I dismiss your review to have a fast merge for this one.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[manstis]

LGTM 👍 @mabulgu

Nice tests.