[Improvement-18286][ApiServer] Remove the default workerGroup from the frontend and add backend validation for workerGroup#18293
Conversation
…dation for workerGroup
| * @param workerGroups worker group names to validate | ||
| * @throws ServiceException if any worker group is not assigned | ||
| */ | ||
| void validateWorkerGroupsAssignedToProject(Long projectCode, List<String> workerGroups); |
There was a problem hiding this comment.
duplicated with WorkerGroupValidator
There was a problem hiding this comment.
duplicated with
WorkerGroupValidator
Okay, I will unify the logic.
| private void validateTaskWorkerGroups(long projectCode, List<TaskDefinitionLog> taskDefinitionLogs) { | ||
| if (CollectionUtils.isEmpty(taskDefinitionLogs)) { | ||
| return; | ||
| } | ||
|
|
||
| List<String> workerGroups = taskDefinitionLogs.stream() | ||
| .map(TaskDefinitionLog::getWorkerGroup) | ||
| .filter(StringUtils::isNotEmpty) | ||
| .distinct() | ||
| .collect(Collectors.toList()); | ||
|
|
||
| projectWorkerGroupRelationService.validateWorkerGroupsAssignedToProject(projectCode, workerGroups); | ||
| } |
There was a problem hiding this comment.
This kind of code should put into WorkerGroupValidator
There was a problem hiding this comment.
This kind of code should put into
WorkerGroupValidator
Okay, I will unify the logic.
SbloodyS
changed the title
| .selectOne(new QueryWrapper<ProjectPreference>().lambda().eq(ProjectPreference::getProjectCode, | ||
| projectCode)); | ||
|
|
||
| // Validate workerGroup is assigned to project |
There was a problem hiding this comment.
| // Validate workerGroup is assigned to project |
| } | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Failed to parse preferences JSON: {}", preferences, e); |
There was a problem hiding this comment.
throw
ServiceExceptionhere.
ok
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Failed to parse preferences JSON: {}", preferences, e); |
|
|
||
| if (StringUtils.isNotBlank(workerGroup) | ||
| && !projectWorkerGroupRelationService.isWorkerGroupAssignedToProject(projectCode, workerGroup)) { | ||
| log.warn("Worker group {} is not assigned to project {}", workerGroup, projectCode); |
SbloodyS
left a comment
SbloodyS
left a comment
There was a problem hiding this comment.
CI failed. Please check it. @njnu-seafish
|
@ruanwenjun @SbloodyS Hi experts, I have a question... Does the default worker group also need to be manually assigned to projects by an admin? Personally, I think it would be better for backward compatibility if the default worker group were assigned to all projects automatically. |
The default workgroup is not very useful. Most users will authorize new workgroups separately. I think it can be removed. |
+1, In fact, we should not set any built-in workgroups by default; all workgroups should be created and assigned manually by users, rather than having all workers registered to a “default” workgroup by default. At #17963, I try to persistent the default worker group, this might be more easily accepted. |
|
…heduler into Improvement-18286
…al mask interception
4 participants
Was this PR generated or assisted by AI?
Yes, Modify the frontend code using AI, then test and verify locally.
Purpose of the pull request
close #18286
Brief change log
Remove the default workerGroup from the frontend and add backend validation for workerGroup
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
Pull Request Notice
Pull Request Notice
If your pull request contains incompatible change, you should also add it to
docs/docs/en/guide/upgrade/incompatible.md