Added support for group attribute in update pipeline config api #7155

kritika-singh3 · 2019-10-30T06:23:52Z

group attribute is mandatory for a PUT request
if the group name is different, a move operation will be performed

- group attribute is mandatory for a PUT request - if the group name is different, a move operation will be performed

varshavaradarajan · 2019-10-30T06:47:34Z

server/src/main/java/com/thoughtworks/go/config/update/UpdatePipelineConfigCommand.java

+        if (!StringUtils.equalsIgnoreCase(group, updatedGroupName)) {
+            PipelineConfigs group = cruiseConfig.findGroup(this.group);
+            group.remove(pipelineConfig);
+            cruiseConfig.getGroups().addPipeline(updatedGroupName, pipelineConfig);


So, this might be a little troublesome. The config save flow for entity enters a synchronized block in https://github.com/gocd/gocd/blob/master/server/src/main/java/com/thoughtworks/go/config/GoConfigDao.java#L88, which means that a group might be deleted before the thread for this request acquires the write lock.

Another thing is that the addPipeline method creates a group if the group name is not found. Is that intentional behaviour? If so, please add a test for the same.

The config save flow for entity enters a synchronized block in https://github.com/gocd/gocd/blob/master/server/src/main/java/com/thoughtworks/go/config/GoConfigDao.java#L88, which means that a group might be deleted before the thread for this request acquires the write lock.

That is a case I missed. Would adding a group existence check in canContinue handle it or would I need to add a check over here itself?
Update: there is a check of canEditPipeline in UpdatePipelineConfigCommand#canContinue which will blow up if the pipeline group does not exist.

Another thing is that the addPipeline method creates a group if the group name is not found. Is that intentional behaviour? If so, please add a test for the same.

Yes, it is intentional (the same happens in CREATE). I have added a test UpdatePipelineConfigCommandTest.java#shouldUpdateThePipelineGroupIfDifferent. Probably would rename it better (Edit: renamed to shouldAddGroupWithPipelineAndRemoveItFromPreviousIfTheSpecifiedGroupDoesNotExist).

That is a case I missed. Would adding a group existence check in canContinue handle it or would I need to add a check over here itself?
Update: there is a check of canEditPipeline in UpdatePipelineConfigCommand#canContinue which will blow up if the pipeline group does not exist.

I suggest moving this block to the update method in UpdatePipelineConfigCommand.java. Anything that updates the cruise config is best added in the entity command's update method. Similar to how CreatePipelineConfigCommand#update does it.

renamed to shouldAddGroupWithPipelineAndRemoveItFromPreviousIfTheSpecifiedGroupDoesNotExist

Assert on group not being there before the update and assert on group being created after the update. That makes it clear that this behaviour is intentional.

@varshavaradarajan I took a deeper look into how the Update pipeline config and delete pipeline group works. As far as I understood, if a group is deleted that means it is an empty group and therefore the pipeline in it would have been either deleted or moved to some other group.
In the 1st case, a RecordNotFoundException would be thrown by the controller. For the 2nd case, the etag comparison should fail. So, I think we are in safe place over here to directly access the group.
WDYT?

cc:\ @maheshp

@kritika-singh3 - Depends actually. You're considering just a delete pipeline group request -
Supposing there's a create pipeline group request that is scheduled to be served right after this operation occurs. And suppose that the group name is the same as the new group name used in the update pipeline config API, then the create call (even though it enters the synchronized block before the update pipeline config API request), will error out because a group with that name already exists.

There are always such unforeseen situations when updating the cruise config without acquiring the write lock. So, it is better to perform anything related to updating the cruise config in the entity's update command.

Edit: I see you have updated the update method. Thanks! Sorry.

- removed entityHashingService#md5ForEntity for PipelineConfig as the etag should now include group name - refactored the UpdatePipelineConfigCommand for readability - fixed tests

maheshp · 2019-10-31T07:56:56Z

server/src/main/java/com/thoughtworks/go/config/update/UpdatePipelineConfigCommand.java

@@ -74,11 +82,11 @@ public boolean canContinue(CruiseConfig cruiseConfig) {
    }

    private boolean canEditPipeline() {
-        return goConfigService.canEditPipeline(pipelineConfig.name().toString(), currentUser, result, getPipelineGroup());
+        return goConfigService.canEditPipeline(pipelineConfig.name().toString(), currentUser, result, getExistingPipelineGroupName());


We need to check if user is admin of the new group to add the pipeline right?

Yes, we need to do. Will add that in the update method of the Command.

…e supplied (if different from the existing one)

Added support for group in update pipeline config api

7e16337

- group attribute is mandatory for a PUT request - if the group name is different, a move operation will be performed

kritika-singh3 added apis pipeline configuration labels Oct 30, 2019

kritika-singh3 added this to the Release 19.10.0 milestone Oct 30, 2019

kritika-singh3 changed the title ~~Added support for group in update pipeline config api~~ Added support for group attribute in update pipeline config api Oct 30, 2019

varshavaradarajan reviewed Oct 30, 2019

View reviewed changes

kritika-singh3 mentioned this pull request Oct 30, 2019

Updated the changelog and pipeline config apis jsons gocd/api.go.cd#485

Closed

kritika-singh3 added 2 commits October 30, 2019 14:40

Updated the etag generation to consider group name as well

524745d

Changes:

a2e9c09

- removed entityHashingService#md5ForEntity for PipelineConfig as the etag should now include group name - refactored the UpdatePipelineConfigCommand for readability - fixed tests

maheshp reviewed Oct 31, 2019

View reviewed changes

Added validation for checking if the user has access to the group nam…

25fd313

…e supplied (if different from the existing one)

kritika-singh3 merged commit 44d61cc into gocd:master Oct 31, 2019

kritika-singh3 deleted the update-pipeline-api-update branch October 31, 2019 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for group attribute in update pipeline config api #7155

Added support for group attribute in update pipeline config api #7155

kritika-singh3 commented Oct 30, 2019 •

edited

Loading

varshavaradarajan Oct 30, 2019

kritika-singh3 Oct 30, 2019 •

edited

Loading

varshavaradarajan Oct 30, 2019

varshavaradarajan Oct 30, 2019

kritika-singh3 Oct 31, 2019

varshavaradarajan Oct 31, 2019 •

edited

Loading

maheshp Oct 31, 2019

kritika-singh3 Oct 31, 2019

Added support for group attribute in update pipeline config api #7155

Added support for group attribute in update pipeline config api #7155

Conversation

kritika-singh3 commented Oct 30, 2019 • edited Loading

varshavaradarajan Oct 30, 2019

Choose a reason for hiding this comment

kritika-singh3 Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

varshavaradarajan Oct 30, 2019

Choose a reason for hiding this comment

varshavaradarajan Oct 30, 2019

Choose a reason for hiding this comment

kritika-singh3 Oct 31, 2019

Choose a reason for hiding this comment

varshavaradarajan Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

maheshp Oct 31, 2019

Choose a reason for hiding this comment

kritika-singh3 Oct 31, 2019

Choose a reason for hiding this comment

kritika-singh3 commented Oct 30, 2019 •

edited

Loading

kritika-singh3 Oct 30, 2019 •

edited

Loading

varshavaradarajan Oct 31, 2019 •

edited

Loading