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

Use BOM and dependabot #37

Merged
merged 9 commits into from
Feb 7, 2021
Merged

Use BOM and dependabot #37

merged 9 commits into from
Feb 7, 2021

Conversation

rantoniuk
Copy link
Contributor

No description provided.

jglick
jglick previously requested changes Oct 27, 2020
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.

Several mistakes.

<relativePath />
</parent>

<artifactId>ssh-agent</artifactId>
<version>1.21-SNAPSHOT</version>
<version>${revision}${changelist}</version>
Copy link
Member

Choose a reason for hiding this comment

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

Did you use mvn incrementals:incrementalify? You forgot to commit the .mvn/ directory if so.

Copy link
Member

Choose a reason for hiding this comment

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

Still missing .mvn/extensions.xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to add that one file instead of commenting 2y old already merged PR ;-)
At the same time, might be just as well good to consider adding this file globally to all repositories under /jenkinsci org - I don't think it would harm them even if they're not using incrementals at the moment?

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to add that one file

Already done in #85.

consider adding this file globally to all repositories

Could, but probably more confusing than helpful, since mvn incrementals:incrementalify expects to add it.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@jglick jglick dismissed their stale review October 27, 2020 13:49

addressed, I think (use of force pushes makes it hard to tell what changed since last review)

rantoniuk and others added 2 commits October 27, 2020 14:55
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
.github/dependabot.yml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@rantoniuk
Copy link
Contributor Author

@jglick thanks for help on this. What's next?

Iis the <maintainers> pom field still used across plugins and up2date in this case? Looking at the merge history looking at the people who merged, it looks to me that there is a de-sync between maintainers and actual people that have repo merge rights.
After all, CI passed and there is an approval from you (so one of the people having merge rights) - wouldn't that work if in such case an auto-merge is done by the pipeline?

As a contributor, I'm just wondering if we can speed up the PR merging process somehow as it's not clear what happens here next.

@jglick
Copy link
Member

jglick commented Oct 28, 2020

POM maintainers can be ignored. Maybe @MRamonLeon is maintaining these days?

@rantoniuk
Copy link
Contributor Author

@MRamonLeon any thoughts?

@rantoniuk
Copy link
Contributor Author

@MRamonLeon, @jglick ping again 🙏

@MRamonLeon MRamonLeon merged commit 312681d into jenkinsci:master Feb 7, 2021
@MRamonLeon
Copy link
Contributor

I don't see any reason to release these changes. Do you need it for any reason? Thank you for your contribution @warden

@rantoniuk
Copy link
Contributor Author

@MRamonLeon thanks for acting on this. No, this one is not needed to be released, more important is #30.

@MRamonLeon
Copy link
Contributor

I've commented on #30 @warden

@jglick jglick mentioned this pull request May 10, 2022
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