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

[JENKINS-27026] - Update SSHD Module from 2.0 to 2.3 #3111

Merged
merged 1 commit into from Dec 10, 2017

Conversation

4 participants
@oleg-nenashev
Member

oleg-nenashev commented Oct 26, 2017

See JENKINS-27026.

Proposed changelog entries

Other fixes are either reverted or too minor

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees @Wadeck

@oleg-nenashev oleg-nenashev requested review from jglick and Wadeck Oct 26, 2017

@jglick

This comment has been minimized.

Member

jglick commented Oct 26, 2017

Actually this is the full diff IIUC.

@jglick

jglick approved these changes Oct 26, 2017

@reviewbybees

This comment has been minimized.

reviewbybees commented Oct 26, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 26, 2017

Actually this is the full diff IIUC.

My bad, fixed the description

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented Oct 27, 2017

@oleg-nenashev Depending on the result of the discussion on Jenkins-core#3074, the SSHD#22 would need some modifications.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 27, 2017

@Wadeck Do you prefer to put the thing on-hold now?

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented Oct 27, 2017

@oleg-nenashev for the moment @jglick and you approved the changes in SSHD, so we can move forward for that part. In the case the discussion goes to another solution, we can just make a new PR for this module, are you OK with that approach ?

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 27, 2017

@Wadeck If there is a risk of revert/breaking change, I would rather put it on hold. If it's only about minor non-breaking improvements, I am fine with merge

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented Oct 27, 2017

@oleg-nenashev at the moment I cannot say, it depends on the discussion :) so yes, put it on hold and we will see.

@jglick jglick added on-hold and removed needs-review labels Oct 27, 2017

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented Oct 30, 2017

As discussed with @oleg-nenashev and @daniel-beck, we will let the SecurityListener as is for the moment and put the re-design into the backlog (aside with Acegi rework).

As a new API for the events will take long to be adopted, it's better to have a short term solution that work (potentially not perfectly but that's fine) and think more about the design of the "future".

=> @jglick are you OK with that approach ?

@jglick

This comment has been minimized.

Member

jglick commented Oct 30, 2017

put the re-design into the backlog

Well, no. Either you introduce new events now, or we do not do them ever.

a new API for the events will take long to be adopted

Not necessarily. Depends on what the listener implementations are. The whole point of introducing new event types would be that existing implementations may not want or expect to be notified for new reasons.

@Wadeck

Wadeck approved these changes Dec 7, 2017

🐝 as for API Token, this one is accepted now

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Dec 7, 2017

@reviewbybees done
@jglick @Wadeck Should I 🚢 🇮🇹 ?

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented Dec 7, 2017

@oleg-nenashev for me it's ok :)

@oleg-nenashev oleg-nenashev merged commit 59b9658 into jenkinsci:master Dec 10, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment