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

Rely on data-binding to (re)configure descriptor #39

Merged
merged 8 commits into from
Aug 30, 2019
Merged

Rely on data-binding to (re)configure descriptor #39

merged 8 commits into from
Aug 30, 2019

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Nov 23, 2017

rely on @DataBoundSetter annotations for data-binding with the global.jelly web-UI.
prefer a nested databound component vs a boolean pseudo-property (useSMTPAuth)

as Descriptor is re-configured, and not created from scratch by data-binding, need to reset attributes to default values.

Those changes are required by jenkinsci/configuration-as-code-plugin#2

<div>
当发送邮件时使用SMTP认证.如果你的环境需要使用SMTP认证,在这里指定其用户名和密码.
<div>
当发送邮件时使用SMTP认证.如果你的环境需要使用SMTP认证,在这里指定其用户名和密码.
Copy link
Member

Choose a reason for hiding this comment

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

Do I dare ask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. local git tool only reports file being moved, no internal changes.

private Secret smtpAuthPassword;

@DataBoundConstructor
public SMTPAuthentication(String smtpAuthUsername, Secret smtpAuthPassword) {
Copy link
Member

Choose a reason for hiding this comment

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

The smtpAuth prefix on these property names is redundant. Better to switch to just username & password.


Use\ SMTP\ Authentication=Verwende SMTP Authentifizierung
User\ Name=Benutzername
Password=Kennwort
Copy link
Member

Choose a reason for hiding this comment

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

I think you neglected to remove these keys from Mailer/global_*.properties.

jenkinsci/jenkins/core/move-l10n.groovy is a useful tool that could perhaps be converted to a mojo in org.jvnet.localizer:maven-localizer-plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I indeed didn't know about this script (and others).

@AndreaGiardini
Copy link

@jglick Hi! Can we push this pull-request forward? It is really necessary in order to use configuration-as-code with the mailer plugin, and since this plugin is very popular it would be nice to have it :)

Thank you

@AndreaGiardini
Copy link

@jglick @ndeloof @kohsuke Sorry to bother you again, can somebody review this pull request? :)

@canhanhan
Copy link

This caused issues with the send test e-mail feature. It was failing at form validation due to field names being different. I sent a PR to the source fork: https://github.com/ndeloof/mailer-plugin/pull/1

@gomesp
Copy link

gomesp commented Oct 20, 2018

It seems that the upstream PR has been merged. Does it mean that the example at https://github.com/jenkinsci/configuration-as-code-plugin/tree/master/demos/mailer now works (or do we still need this PR to be merged as well)?

@QuentinBisson
Copy link

QuentinBisson commented Oct 22, 2018

@jglick @ndeloof @kohsuke Sorry to bother you about this. Any news on when it's gonna be merged ? I need the possibility to add authentication from configuration as code plugin

@oleg-nenashev
Copy link
Member

I have no bandwidth to review it this week. Everybody is busy at Jenkins World, I believe

@alecharp alecharp self-requested a review December 3, 2018 08:50
@darxriggs
Copy link

ping

Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

@jglick I was looking at this PR and wondering how the data migration to the SMTPAuthentication is done. I'll test it locally by wondering if you could confirm that it's what the lines https://github.com/jenkinsci/mailer-plugin/pull/39/files#diff-e64d5e9d7b997b028696a748431563ceR387 are about. Thanks

@jglick
Copy link
Member

jglick commented Mar 1, 2019

I am afraid I have no memory of this PR. Needs a maintainer.

@przecze
Copy link

przecze commented Mar 6, 2019

As mentioned before, I'm also looking forward to this PR being merged, so I can use JCasC for mailer.

# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

Choose a reason for hiding this comment

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

@ndeloof Why does this properties file contain 13 properties whereas the other ones only contain 3?

@@ -30,8 +30,5 @@ Default\ user\ e-mail\ suffix=Suffixe par d\u00E9faut des emails des utilisateur
Sender\ E-mail\ Address=Adresse e-mail de l''''exp\u00E9diteur

Choose a reason for hiding this comment

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

In l''''exp\u00E9diteur there are two extra '' which should be removed.

@@ -0,0 +1,22 @@
# The MIT License

Choose a reason for hiding this comment

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

@ndeloof Why is this empty "global.properties" file added for Mailer but no "config.properties" for SMTPAuthentication?


<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<f:entry title="${%User Name}" field="username">
Copy link

@darxriggs darxriggs Mar 10, 2019

Choose a reason for hiding this comment

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

@ndeloof Indentation (lines 27-32) should be corrected from 6 to 2.

@@ -20,18 +20,15 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

E-mail\ Notification=Notificaci�n por correo electr�nico
E-mail\ Notification=Notificaci�n por correo electr�nico

Choose a reason for hiding this comment

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

@ndeloof Just looking at the diff on GitHub it looks like the encoding got broken. Could you double-check this (this and other properties files)?

@@ -25,9 +25,7 @@ E-mail\ Notification=E-mail p\u00e5mindelser
Default\ user\ e-mail\ suffix=Standard e-mail endelse
Use\ SSL=Brug SSL
SMTP\ Port=SMTP Port
Use\ SMTP\ Authentication=Benyt SMTP Authentificering

Choose a reason for hiding this comment

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

@ndeloof Did you miss to remove Password in line 23?

@@ -25,9 +25,7 @@ E-mail\ Notification=E-mail p\u00e5mindelser
Default\ user\ e-mail\ suffix=Standard e-mail endelse
Use\ SSL=Brug SSL
SMTP\ Port=SMTP Port
Use\ SMTP\ Authentication=Benyt SMTP Authentificering

Choose a reason for hiding this comment

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

@ndeloof "${%Use SMTP Authentication}" is still used in "Mailer/global.jelly".
Is it correct to move it from "Mailer/global*.properties" to "SMTPAuthentication/config*.properties"?

@darxriggs
Copy link

@ndeloof ping

@jglick
Copy link
Member

jglick commented Apr 3, 2019

I do not think @ndeloof is working on this any more. Would fall to whoever is driving configuration-as-code these days. CC @ewelinawilkosz

@darxriggs
Copy link

@alecharp do you still have this on your list as mentioned in the Jira issue?

@alecharp
Copy link
Member

alecharp commented Apr 14, 2019 via email

@darxriggs
Copy link

Thank you very much in advance.

@bobbui
Copy link

bobbui commented Jun 27, 2019

any update guys?

@pietroaragona
Copy link

Any news about this PR? It is very useful when use JCAC plugin
Thanks

@h1dden-da3m0n
Copy link

h1dden-da3m0n commented Aug 5, 2019

Okay, sorry to make this note here but I believe it will help a few people that find this PR based on the JCasC demos, which state setting SMTP doesn't work.
It dose now, or better since the mailer plugin version 1.22 issue, pr).
I'll try and get a PR in with the JCasC demos to fix the statement that it doesn't work.

NOTE: I do not claim to be familiar with this PR or what it intends to change/fix so it might still be useful/necessary to fix other issues. I only posted this here since I got redirected here and just by accident found out that setting SMTP already works now and thought other people might have that same "problem".

@bobbui
Copy link

bobbui commented Aug 5, 2019

Okay, sorry to make this note here but I believe it will help a few people that find this PR based on the JCasC demos, which state setting SMTP doesn't work.
It dose now, or better since the mailer plugin version 1.22 issue, pr).
I'll try and get a PR in with the JCasC demos to fix the statement that it doesn't work.

NOTE: I do not claim to be familiar with this PR or what it intends to change/fix so it might still be useful/necessary to fix other issues. I only posted this here since I got redirected here and just by accident found out that setting SMTP already works now and thought other people might have that same "problem".

awesome, looking forward to it.
thanks

@fcojfernandez
Copy link
Contributor

Superseded by #61 which intends to land this PR, which seems to be stalled and with no progress. That PR also fixes the test failure in the PR-builder and adds tests to check the compatibility with JCasC.

@alecharp you can close this PR as your convenience.

@alecharp alecharp merged commit 1903d21 into jenkinsci:master Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet