search

jenkinsci / plasticscm-plugin

A plugin for Jenkins to be able to use Plastic SCM
MIT License
15 stars 31 forks source link

Only run cm update with branch selector #35

Closed mig42 closed 4 years ago

mig42 commented 4 years ago

As reported by @ahatala in PR #33 and issue #34, the checkout action updates workspaces even if they're switched to a changeset. In those scenarios, the Plastic SCM plugin shouldn't update to the latest changeset in the branch of the selector changeset.

That change of behavior came from the update command in Plastic SCM CLI.

This PR modifies the behavior of the checkout action to skip the update action if the current selector includes the word 'changeset' or 'label'. The update will still run if you change the selector, no matter what's in it.

Resolves #34

knapsu commented 4 years ago

Hi @mig42. It seems I did not get any notifications from GitHub about any bug reports or pull requests. Clicked "Watch everything" so I will not miss those again.

I've actually had a fix for this in my own git but never synchronized it with GitHub. Will combine your changes with mine.

On the technical side SelectorNeedsUpdate(...) method name does not follow Java camel case standard and is too similar in name to mustUpdateSelector(...) method.

mig42 commented 4 years ago

@knapsu Oh, good point! I think what you mean is that the updateWorkspace() call in line 78 shouldn't run either, right?

https://github.com/jenkinsci/plasticscm-plugin/blob/ae2b66e42653af7c6ea4baaaaf9b4732f49a282d/src/main/java/com/codicesoftware/plugins/hudson/actions/CheckoutAction.java#L74-L80

I think it just needs the same if (isBranchSelector(tool, selector) call above, right?

I'm aware that the code in PR #30 is a lot more complete that this one (I really appreciate that, by the way!) - it's just that I'd like to publish these changes quickly to solve the original issue and then allocate some time so I can review and validate your PR the way it deserves.

knapsu commented 4 years ago

It is not as straight as just adding if (isBranchSelector(tool, selector)) on the specified line. The real problem that was causing the issue is cm update (WorkspaceManager.updateWorkspace(...)) rewriting the selector file and loosing the 'changeset "..."` part.

I believe cm setselector which actually also triggers workspace update is a must have to cover all the situations. This is exactly what I do in PR #30. Jenkins plugin is bound to using selectors unless those will be deprecated in favor of CsetSpec but this is a bigger task.

mig42 commented 4 years ago

But if we don't run that WorkspaceManager.updateWorkspace(...) for cset/label selectors we wouldn't have the issue, right?

Regarding the setselector, I'm not 100% sure that it triggers the update if the selector doesn't change and that's why we update the workspace in that case...

knapsu commented 4 years ago

If you don't run updateWorkspace(...) after creating new workspace you will be at cs:0.

I've tested setSelector(...) and I am certain that it will always cause workspace update after switching the selector, even if the selector stays the same.

knapsu commented 4 years ago

It looks good now with one exception. Workspace cleanup command should not be run on newly created workspace as there is no need for it.

Please take a look at this for reference https://github.com/jenkinsci/plasticscm-plugin/blob/0bd5073e84b9b523f7bdcc0502c2dcf4afd60377/src/main/java/com/codicesoftware/plugins/hudson/actions/CheckoutAction.java#L36-L52

mig42 commented 4 years ago

@knapsu Yes, you were right 😊 I just pushed my local repo before I had finished working in the task. It should be OK now.