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-59320 - Should log all running jobs when bundle is generated #189

Merged
merged 18 commits into from Sep 19, 2019
Merged

JENKINS-59320 - Should log all running jobs when bundle is generated #189

merged 18 commits into from Sep 19, 2019

Conversation

Evildethow
Copy link
Contributor

@Evildethow Evildethow self-assigned this Sep 13, 2019
@varyvol
Copy link

varyvol commented Sep 13, 2019

I think the anonymization concerns that were raised in the previous PR still apply.

@aheritier
Copy link
Member

yes good point @varyvol URLs should be anomymized @Evildethow if the feature is active

jglick
jglick previously requested changes Sep 13, 2019
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Also, anonymization as mentioned by others.

src/main/java/com/cloudbees/jenkins/support/impl/RunningJobs.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/support/impl/RunningJobs.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/support/impl/RunningJobs.java Outdated Show resolved Hide resolved
src/test/java/com/cloudbees/jenkins/support/impl/RunningJobsTest.java Outdated Show resolved Hide resolved
@Evildethow
Copy link
Contributor Author

Evildethow commented Sep 15, 2019 •
edited

@jglick @MRamonLeon thanks for the review. I've made the changes to the best of my ability.

@aheritier To get this working w/ Pipeline the base version had to move from 2.107 -> 2.138.4, and we also need to pull in workflow-durable-task-step as an optional dep.

Thanks, all

aheritier
aheritier approved these changes Sep 16, 2019
View reviewed changes
Copy link
Member

@aheritier aheritier left a comment

Choose a reason for hiding this comment

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

It looks good to me - excepted my question on the null check (but I'm confident that you had a good reason for the case)

src/main/java/com/cloudbees/jenkins/support/impl/AboutJenkins.java Show resolved Hide resolved
jglick
jglick reviewed Sep 16, 2019
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/support/impl/RunningJobs.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/support/impl/RunningJobs.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/support/impl/RunningJobs.java Outdated Show resolved Hide resolved
src/test/java/com/cloudbees/jenkins/support/impl/RunningJobsTest.java Outdated Show resolved Hide resolved
src/test/java/com/cloudbees/jenkins/support/impl/RunningJobsTest.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/support/impl/RunningJobs.java Outdated Show resolved Hide resolved
@Evildethow
Copy link
Contributor Author

Thanks for the review, @jglick

@Evildethow
Copy link
Contributor Author

Requested changes made. @aheritier I think this one is good to go.

oleg-nenashev
oleg-nenashev reviewed Sep 18, 2019
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Plugin dependencies could be also cleaned up after moving to BOM

@Evildethow
Copy link
Contributor Author

Plugin dependencies could be also cleaned up after moving to BOM

Understood, but I'd rather address that separately. I'm considering it out-of-scope for now.

@aheritier
Copy link
Member

yes let's cleanup the deps in another PR @Evildethow

@Evildethow
Copy link
Contributor Author

As there seem to be no further requests for changes I'm going to merge this.

Thanks, all.

@Evildethow Evildethow merged commit ed52e4e into jenkinsci:master Sep 19, 2019
@Evildethow Evildethow deleted the feature2/JENKINS-59320 branch September 19, 2019 21:29
@Evildethow
Copy link
Contributor Author

I've created https://issues.jenkins-ci.org/browse/JENKINS-59453 to address migrating to the BOM

@Evildethow Evildethow mentioned this pull request Sep 29, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick left review comments

@oleg-nenashev oleg-nenashev oleg-nenashev left review comments

@varyvol varyvol varyvol left review comments

@MRamonLeon MRamonLeon MRamonLeon left review comments

@fcojfernandez fcojfernandez fcojfernandez left review comments

@aheritier aheritier aheritier approved these changes

@Dohbedoh Dohbedoh Awaiting requested review from Dohbedoh

Assignees

@Evildethow Evildethow

Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@Evildethow @varyvol @aheritier @jglick @oleg-nenashev @MRamonLeon @fcojfernandez