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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-30435][JENKINS-30436] Updating proxies on template project rename and non-existing project name validation #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[JENKINS-30435][JENKINS-30436] Updating proxies on template project rename and non-existing project name validation #27

wants to merge 2 commits into from

Conversation

oleksandr-horobets
Copy link

No description provided.

oleg-nenashev
oleg-nenashev reviewed Dec 12, 2015
View reviewed changes
src/main/java/hudson/plugins/templateproject/ItemListenerImpl.java

boolean updatedItems = false;

if(project instanceof Project){
Copy link
Member

Choose a reason for hiding this comment

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

Bad approach. There may be other implementations of abstract project, which are being ignored here

Copy link
Author

Choose a reason for hiding this comment

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

I don't like this approach too, but how can I obtain project builders using only AbstractProject?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I would recommend to add a logging on the INFO level in such case. This approach may become obsolete when Template projects start referencing their users somehow.

@jenkinsadmin
Copy link
Member

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

@Brantone
Copy link
Contributor

Brantone commented Feb 6, 2016

@oleksandr-horobets : Will some of these recommendations be implemented?

@Brantone
Copy link
Contributor

Still un-implemented and now with conflicts to resolve :(
The tests are nice, but lots of recommendations that should be corrected

@oleksandr-horobets
Copy link
Author

@Brantone I'm working on comments now, hope will be done soon:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oleksandr-horobets @jenkinsadmin @Brantone @oleg-nenashev