Ruigao/list disappeared vc jobs #112
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for tracking and using historical Virtual Cluster (VC) access for users. When users query their jobs, they can now see jobs from VCs they previously had access to, not just their current VCs.
Key changes:
- Added
history_vclistfield to user schema to store historical VC access - Implemented
getHistoryVCs()function that retrieves VCs from user's job history and group memberships - Modified job listing logic to include historical VCs alongside current VCs
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rest-server/src/utils/manager/user/user.js | Added history_vclist field to user schema validation |
| src/rest-server/src/utils/manager/user/crudK8sSecret.js | Implemented getHistoryVCs() function and updated create/read/update operations to populate and persist historical VC list |
| src/rest-server/src/models/v2/job/k8s.js | Added listVCsFromJob() function to retrieve unique VCs from a user's job history |
| src/rest-server/src/controllers/v2/user.js | Added getUserHistoryVCs() controller function and initialized history_vclist in user creation |
| src/rest-server/src/controllers/v2/job.js | Updated job list logic to merge current and historical VCs for filtering |
Comments suppressed due to low confidence (1)
src/rest-server/src/controllers/v2/job.js:53
- Variable currentvcs is used like a local variable, but is missing a declaration.
currentvcs = await userController.getUserVCs(username);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| return await groupModel.getGroupVCs(group); | ||
| } catch (error) { | ||
| console.error(`Failed to fetch VCs for group ${group}:`, error); |
There was a problem hiding this comment.
Use the logger instance instead of console.error for consistency with the rest of the codebase. Change to logger.error(...).
| console.error(`Failed to fetch VCs for group ${group}:`, error); | |
| logger.error(`Failed to fetch VCs for group ${group}:`, error); |
|
|
||
| const username = req[userProperty].username; | ||
| myvcs = await userController.getUserVCs(username); | ||
| currentvcs = await userController.getUserVCs(username); |
There was a problem hiding this comment.
Missing variable declaration keyword. Add const or let before currentvcs to avoid creating an implicit global variable.
| currentvcs = await userController.getUserVCs(username); | |
| let currentvcs = await userController.getUserVCs(username); |
| } | ||
| else { |
There was a problem hiding this comment.
[nitpick] The else should be on the same line as the closing brace of the if statement for consistency with JavaScript style conventions: } else {
| } | |
| else { | |
| } else { |
| res.json(data); | ||
| }); | ||
|
|
||
| const get = asyncHandler(async (req, res) => { |
There was a problem hiding this comment.
the other api includes: get/update/getConfig/getevents//getlogs still no access controller, they're independent with listjobs
| 'userName', | ||
| 'virtualCluster', | ||
| ], | ||
| where: { userName: username }, |
There was a problem hiding this comment.
since it's a database, is it possible to select virtualcluster directly?
| return await groupModel.getGroupVCs(group); | ||
| } catch (error) { | ||
| console.error(`Failed to fetch VCs for group ${group}:`, error); | ||
| return []; // Return an empty array on failure |
| const vcSet = new Set(); | ||
| const vcPromises = grouplist.map(async group => { | ||
| try { | ||
| return await groupModel.getGroupVCs(group); |
There was a problem hiding this comment.
why history vc get from group info，is it get current vcs or get history vcs from secret?
| async function getHistoryVCs(name, grouplist, retrieveFromHistory=true) { | ||
| // Retrieve VC list from the user's job history | ||
| let vcsFromJob = []; | ||
| if (retrieveFromHistory) { |
There was a problem hiding this comment.
so if not retrieveFromHistory, if will only return the vcs in getGroupVCs? it should return original historyvcs in secret + getGroupVCs?
|
and here is a question, if a user had one vc, but currently the vcs still work, but admin delete this user from this vc, under current design, it still has access to this vc's jobs, this seems not make sense |
List user's history jobs which in the virtual clusters that have been removed from the service.