-
Notifications
You must be signed in to change notification settings - Fork 881
Workspace ID should be Workspace Name on build log page #1801
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
Comments
One thing I observe here is that we only get the workspace id in the API call. So there are 2 options:
Which is preferable @f0ssel ? {
"id": "9c01ef71-7342-4150-bd2b-6b7d40437a33",
"created_at": "2022-05-26T20:41:47.793394Z",
"updated_at": "2022-05-26T20:41:56.783249Z",
"workspace_id": "bef92599-a895-44a6-bd0c-9210527d07e6",
"template_version_id": "56a2197e-599e-4f50-80aa-9510620f1de4",
"build_number": 2,
"name": "ecstatic_mirzakhani1",
"transition": "stop",
"initiator_id": "f8ab7401-eaf9-4cf3-8159-ba7b98da57ed",
"job": {
"id": "cde85e77-0622-4ef0-8db5-2170a2425a6b",
"created_at": "2022-05-26T20:41:47.792008Z",
"started_at": "2022-05-26T20:41:47.813741Z",
"completed_at": "2022-05-26T20:41:56.783614Z",
"status": "succeeded",
"worker_id": "c0a53d45-4750-4baf-9b81-20a03145e7e2"
},
"deadline": "0001-01-01T00:00:00Z"
} |
I think it's fine for the frontend to load the workspace from that ID. |
A big problem in v1 for both latency and DB load was recursive queries. Could embedding the workspace or workspace name in the build log let us achieve the issue without the aforementioned problems? |
Recursive queries don't exist anymore, because we bulk requests by ID. eg. |
I think you're talking about loading lists one by one. I'm talking about when loading a page requires a query to further detail a past query result. |
How often are we going to be querying for a workspace and don't want the workspace name? It seems infrequent enough that we shouldn't make what I expect will be a relatively common case (wanting both) to require two steps. Also this doesn't have issues with arbitrary follow-up queries, so I don't think we need to be paranoid about performance impact. This adds exactly one primary key database query per API call. |
I agree with this rationale.
The DB performance hit rarely comes from the obvious, central business query. In v1, most API calls generated 20+ queries related to licensing, authentication, auditing etc. This was probably excessive, but you can see how the number of queries gets out of hand as the middleware stack grows. |
We already seem to have the workspace in these routes, so no extra db calls, just add the field to the api response should be fine. But I love having these questions - I have not seen many "complex" queries like in v1 so at least so far it seems we are doing a better job. |
One interesting question is whether we should modify the |
If it's purely additive, it seems like it shouldn't be harmful to existing consumers. Also we're in pre-release, so we have no obligation to maintain API compatibility. |
Turns out literally everywhere we currently use that type we already have a workspace object in hand, so I'm going with that |
Please add your planning poker estimate with ZenHub @spikecurtis |
Please add your planning poker estimate with ZenHub @f0ssel |
Verified workspace name displayed instead of ID on build log page. |
I think it would be more helpful if we showed the workspace name instead of ID (keep link the same).

The text was updated successfully, but these errors were encountered: