search

jenkinsci / plasticscm-plugin

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

Pipeline support for changeset pull and configurable workspace cleanup #30

Closed knapsu closed 3 years ago

knapsu commented 4 years ago

Fixes and improvements from Housemarque.

knapsu commented 4 years ago

update_cleanup_01

mig42 commented 3 years ago

Hi @knapsu!

I'm sorry, I missed this one 😯 I'll try to review and test it as soon as I can! Thank you for the effort 😊

knapsu commented 3 years ago

@mig42 I've merged master branch. Please review.

knapsu commented 3 years ago

Hi.

I also have a question regarding this:

Fix Workspace directories can now be nested multiple levels deep

What's the use case for that? The Directory setting in the config doesn't allow to specify subdirectories, so I'm not exactly sure about what this is fixing...

This corrects a bug where the plugin would fail if there would be an existing workspace located in for example WORKSPACE/libraries/SDL). You correctly said that using "Directory" field would not allow such thing but there are also custom shell scripts which pull third party libraries to other locations or just to do partial checkout which plugin does not suppor. With this PR it will discover such workspaces and handle those gracefully.

About this:

Improvement Prevent cluttering console output when retrieving changesets history

This issue went away with PRs #36 and #37, so it might be a good idea to either remove it from the changes list or even remove it completely.

This is a different thing. #36 and it's fix in #37 is for command memory usage (which was filled with the big chunks of XML text output). My changes add a possibility for commands to be quiet and skip outputting the Plastic client communication to the console to avoid clutter. For example now retrieving log history between two changesets will be silenced. No one really wants to see XML in the output. They want to see a pretty and neat list of changes in the job summary instead (sometimes just the XML had a couple of megabytes of text - some drastic cases had even 30 MB!).

Finally, as a side note, I think it would be a good idea in the future to submit smaller, multiple PRs (each one with a single improvement/fix) instead of a big one. I believe that we could all benefit from that blush That way it's easier for us to allocate time to review and validate it. This increases the chances that we process PRs earlier and have them merged. It decouples fixes too, so change requests during review don't put on hold a bunch of new features, just one of them. What do you think?

Yup, smaller PRs would be good, but the reality is that when I start working on one I see something broken and just start patching/improving it immediately. This is how from a single thing it becomes more and more. Doing tests is time consuming as I do test not only locally but also on real data/use cases. That is why I prefer to check multiple things with one iteration, because of the time spend on settign up the tests. Also I had few cases where with multiple PR's one would be dependent on fixes provided in earlier PR. And it is impossible to create a PR in GitHub in this situation where necessary changes are not yet in (or is it ?). Even if it would be possible, merging each change in master or other PR's back to every PR that is waiting would be a real burden for me. Either way I will do my best to make PR's smaller to make them easier to digest. :+1: