cleanup: Use policy name to fetch status #1105
Conversation
This uses the policy name in order to fetch the status. This is a lot more user friendly than relying on the ID. To enable this, I added a constraint for a combination of project ID and name to be unique in the database.
It's redundant as we provided the name when getting the status for it
| option (google.api.http) = { | ||
| get: "/api/v1/policy/{policy_id}/status" | ||
| get: "/api/v1/policy/name/{name}/status" |
There was a problem hiding this comment.
Why is it necessary to have name explicitly as part of the path?
There was a problem hiding this comment.
Else the JSON API wouldn't work. We probably want to revisit our API and clean this all up as well
There was a problem hiding this comment.
Wait -- why wouldn't the JSON API work? Was there a conflict?
It feels like we should adjust GetPolicyById and DeletePolicy to use the same name format.
There was a problem hiding this comment.
A few comments, but don't let me stand in the way of progress.
I am somewhat concerned about the patterns in our REST API -- for example, creating a policy is POST /api/v1/policy, while listing the things you just created is GET /api/v1/policies (note the plural). I know that Kubernetes did the pluralization thing and it's really cute, but I think it's probably also fundamentally a mistake (certainly, the Kubernetes layer for getting this right is way more complex than you'd hope for).
| @@ -188,6 +188,8 @@ CREATE TABLE policies ( | |||
| FOREIGN KEY (project_id, provider) REFERENCES providers(project_id, name) ON DELETE CASCADE | |||
| ); | |||
|
|
|||
| CREATE UNIQUE INDEX ON policies(project_id, name); | |||
There was a problem hiding this comment.
I had a preference for UNIQUE (project_id, name), but reading https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-DESC-ADD-TABLE-CONSTRAINT-USING-INDEX suggests that we may need care to use ALTER TABLE ADD UNIQUE (project_id, name) later.
There was a problem hiding this comment.
I seem to recall that previously we implicitly had this through the provider, name index implicitly including the project_id in the provider. Are we intentionally breaking that now? And, do we need defensive code in the application to ensure that provider.project_id is a parent (or self) of project_id?
There was a problem hiding this comment.
@evankanderson I don't see how that breaks, just above you can still see the:
FOREIGN KEY (project_id, provider) REFERENCES providers(project_id, name) ON DELETE CASCADE
| option (google.api.http) = { | ||
| get: "/api/v1/policy/{policy_id}/status" | ||
| get: "/api/v1/policy/name/{name}/status" |
There was a problem hiding this comment.
Wait -- why wouldn't the JSON API work? Was there a conflict?
It feels like we should adjust GetPolicyById and DeletePolicy to use the same name format.
I'm not super keen on those inconsistencies either and I have more concerns on the complete randomness of the API. That's actually what brought my comment from yesterday about downgrading to |
I needed to remove |
|
@evankanderson in the meantime, I'll merge this and will follow up on a more general API cleanup approach. |
This uses the policy name in order to fetch the status. This is a lot
more user friendly than relying on the ID.
To enable this, I added a constraint for a combination of project ID and name
to be unique in the database.