Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p4.prog reporting feature #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jkapica
Copy link
Contributor

@jkapica jkapica commented Apr 8, 2014

This patch provides capability to report custom p4 program name to the Perforce server.

p4.prog is an arbitrary string which identifies the application establishing connection to p4 server. It can be later traced in server logs as well as with p4 monitor. It is very useful if you have several automated systems connecting to p4 server, and you want to be able to easily trace and account them.

Javadoc is pretty scanty about the parameter, below link to python docs:
http://www.perforce.com/perforce/doc.current/manuals/p4script/03_python.html#1120041

@cloudbees-pull-request-builder

plugins » perforce-plugin #94 SUCCESS
This pull request looks good

@jkapica jkapica mentioned this pull request Apr 8, 2014
@oleg-nenashev
Copy link
Member

I definitely vote for such feature.
BTW, the PR follows the extensive approach, because it adds new entries to the monstrous PerforceSCM class. I'd suggest to move such feature to a separate class if there is any possibility of new features/configuration options to be added.

Probably, it would be good to have a P4ProgramProvider extension point (see #45 for a similar approach). It will allow to provide more flexibility and to decouple logic from PerforceSCM as well.

Unfortunately, seems the development of the plugin has been put on hold due to the unclear situation with guys from Perforce, who want to create their own plugin. Probably, @rpetti could provide more information on future actions with the existing plugin.

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@jkapica
Copy link
Contributor Author

jkapica commented Apr 10, 2014

Your proposal is reasonable and it would probably be more elegant, unfortunately I'm not sure when I'll find some time to rewrite it the way you described. From my pov I have a working solution that is "good enough" atm.

I'd be interested to hear about the perforce2 plugin. Maybe there is no point in making this one more elegant, if it's going to be scratched anyway.

* Perforce program name to report to the Perforce server
*/
String p4ProgramName = null;
String p4ProgramPollingName = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving this variables and the logic to a separate Describable class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why not. I do see the benefits of having this as a separate describable class or extension point. I'm just after the functionality.

@oleg-nenashev
Copy link
Member

@jkapica
Do you plan to rework this PR?
The improvement LGTM, but it should be merged with a current version at least

@oleg-nenashev oleg-nenashev added this to the in-progress milestone Nov 5, 2014
@jkapica
Copy link
Contributor Author

jkapica commented Nov 5, 2014

Sorry. After I got it working in my instance, this endded up on end of my priorities list.
I agree that the rework is a good idea, but it may be over my capacity.
TBH, I'm still running old version. Since there are conflicts in this pull request, you are free to reject it. I will review this item again, and create new request when time for update comes. Hopefully sooner rather than later :}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants