[IA-4912] Add DELETE chartVersion endpoint

[lucymcnatt]

Adding the DELETE CRUD Admin endpoint for Sherlock to use to interact with the APP VERSION table

DELETE /api/admin/v1/charts/versions/{chartNames}

Soft-deletes a record in the APP VERSION table (DELETE) (sets their inactive_at datetime)

[jdcanas]

Hmm... do we really want to take a pipedelimited list of parameters as a raw string in the query params? Would it be possible to take a json payload with a proper array in a POST request?

[lucymcnatt]

the possible styles are form, spaceDelimited, pipeDelimited, deepObject

spaceDelimited and pipeDelimited are what you'd expect, this is what the other 2 look like on swagger

Form:
api/admin/v1/charts/versions?chartNames=string1&chartNames=string2
deepObject:
api/admin/v1/charts/versions?chartNames=string1,string2

https://swagger.io/docs/specification/serialization/#:~:text=values%20get%20prefixed.-,Query%20Parameters,-Query%20parameters%20support

[lucymcnatt]

oh wait I see what you were suggesting, to make this a POST to a different endpoint instead of a DELETE?

[jdcanas]

yeah, I think I would prefer that to parsing query parameters. Open to other thoughts though, but it seems preferable to either have it take a JSON payload to delete multiple or to have the delete endpoint take a single chartName for deletion

[lucymcnatt]

cc: @cpate4 what are your thoughts on making this a POST vs. DELETE? or sticking to a single chartName?

[cpate4]

My original thoughts were that we would use this URL form:
DELETE api/admin/v1/charts/versions?chartName=string1&chartName=string2

... and let the framework do "the parsing" for us.

If the hangup is around syntax for deleting multiple, I would recommend just keeping it simple and using @lucymcnatt's original format of:
DELETE api/admin/v1/charts/versions/chartName

Total random aside, and happy for any feedback on this ...
I am seeing the syntax of our URL and wondering if we should just drop the versions/ from the URL, making our URL format:
ACTION api/admin/v1/charts/chartName

[lucymcnatt]

I have to agree on the versions/ point, it feels a little redundant when we already have charts/

and I feel comfortable sticking with Form style (...versions?chartName=string1&chartName=string2), @jdcanas is that ok with you?

[cpate4]

I am tempted to keep it "as is" for now for the following reasons:

1. ... if we did this, would we rename chartVersion to chart with the following structure?

{
  name: ...
  version: ...

  app_version: ...
  ... 
}

2. but ultimately, the app_version is just a param into the chart, and therefore, should we introduce params or options as a map?

... so I go back to, the is a specific case at the moment. I am back to recommending that we leave it, with its current structure, and simplify the endpoint to only delete singles.

[mlilynolting]

this looks good - there are some remaining "chartVersion" references in the code; is there a reason to keep this and not just change everything to "chart"?

[jdcanas]

LGTM, just remove the unnecessary throws exception sonarcube is pointing out

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud