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-22088] - Upgrade Extras Executable War from 1.36 to 1.37 #3223

Merged
merged 1 commit into from Feb 3, 2018

Conversation

6 participants
@oleg-nenashev
Member

oleg-nenashev commented Jan 9, 2018

Bumps the extra executable war and pulls in 2 notable changes.

Proposed changelog entries

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

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 9, 2018

@kuisathaverat

LGTM 🐝

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jan 10, 2018

Could we get a linked for PR for packaging that demonstrates the benefits of the first entry? Otherwise it's just going to be one of those unused features nobody will remember in a few years.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 10, 2018

@daniel-beck Do you propose modifying https://github.com/jenkinsci/packaging ?
I see no immediate benefit there since we do not pass secrets there in the default setup. CC @svanoort

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 10, 2018

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 10, 2018

I forgot to add @arykov to Cc, sorry

@dwnusbaum dwnusbaum self-requested a review Jan 10, 2018

@reviewbybees

This comment has been minimized.

reviewbybees commented Jan 11, 2018

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.

@dwnusbaum

This comment has been minimized.

Member

dwnusbaum commented Jan 11, 2018

@oleg-nenashev We might not pass secrets by default, but variables that should be secret are defined in the config files (i.e. jenkins.sysconfig.in) and then passed as regular command line variables, (i.e. jenkins.init.in). We could update the config file to recommend using paramsFromStdIn for any sensitive variables, what do you think?

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 13, 2018

@dwnusbaum @daniel-beck does Packaging block this change?

We could update the config file to recommend using paramsFromStdIn for any sensitive variables, what do you think?

I totally agree with that, but it should be done in follow-up tickets IMHO. Just because we are landing other changes

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jan 13, 2018

I'd like to see more reviews for this. If it's straightforward it shouldn't be too difficult to get them. I'm still not convinced this actually does something useful without an implementation that makes sense.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 21, 2018

@dwnusbaum

This comment has been minimized.

Member

dwnusbaum commented Jan 22, 2018

@oleg-nenashev I am fine with packaging being updated separately. (Should we go ahead and create a ticket for it?)

I reviewed the linked PR and tested it locally and it seems fine, although I'm not sure how useful the feature will be in general, and I have no expertise on the best practices for handling sensitive parameters in command line tools (e.g., @ prefix to use files as @jglick mentioned in the PR, or the --paramsFromStdin parameter).

I think @daniel-beck's request for additional reviews still stands.

@NicholasAzar

This comment has been minimized.

NicholasAzar commented Jan 23, 2018

@oleg-nenashev Any update on when this can be reviewed or accepted? The project i’m working on is urgently interested in the --paramsFromStdin feature.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 23, 2018

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 26, 2018

@dwnusbaum

🐝 Looks good to me.

@oleg-nenashev oleg-nenashev requested a review from daniel-beck Jan 29, 2018

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 29, 2018

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 29, 2018

@jenkinsci/code-reviewers Would appreciate some extra reviews so that I can address @daniel-beck's comment

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Feb 3, 2018

@jenkinsci/code-reviewers any feedback will be appreciated. If there is no negative feedback, I would like to merge it into the next weekly

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Feb 3, 2018

As discussed with @daniel-beck, I am going to merge that and to handle the follow-up packaging ticket later this month

@oleg-nenashev oleg-nenashev merged commit bff2d2a into jenkinsci:master Feb 3, 2018

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