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

JAMES-2150 Added the ability to upload Sieve scripts via the CLI #1828

Closed

Conversation

matzepan
Copy link

Implemented the steps described in JAMES-2150

@chibenwa chibenwa added the contribution Contribution of a kind contributor - also applicable for community oriented efforts. label Oct 23, 2018
@chibenwa chibenwa added this to the polish_2018_10 milestone Oct 23, 2018
@@ -62,7 +62,8 @@
REMOVESIEVEQUOTA("RemoveSieveQuota"),
GETSIEVEUSERQUOTA("GetSieveUserQuota", "username"),
SETSIEVEUSERQUOTA("SetSieveUserQuota", "username", "quota"),
REMOVESIEVEUSERQUOTA("RemoveSieveUserQuota", "username");
REMOVESIEVEUSERQUOTA("RemoveSieveUserQuota", "username"),
ADDACTIVESIEVESCRIPT("AddActiveSieveScript", "username", "scriptname", "script");
Copy link
Member

Choose a reason for hiding this comment

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

Passing a SIEVE script directly as a bash script parameter seems like a bad idea to me... A sieve script is a multiline, potentially long string.

IMO we should do like in MailboxManagerManagement::importEmlFileToMailbox: rely on the file path rather than on the full string.

@@ -331,6 +331,9 @@ private void executeCommand(String[] arguments, CmdType cmdType, PrintStream pri
case REMOVESIEVEUSERQUOTA:
sieveProbe.removeSieveQuota(arguments[1]);
break;
case ADDACTIVESIEVESCRIPT:

Choose a reason for hiding this comment

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

this case should be indented as the previous one

@chibenwa
Copy link
Member

I am relaunching the CI build on your pull request

test this please

@matzepan
Copy link
Author

I added the ability to load from a file - but I have to say that I am quite confused what the difference between SieveRepositoryManagement and SieveProbeImpl should be. Looks like quite the duplicate functionality.

this.mailboxProbe = mailboxProbe;
this.quotaProbe = quotaProbe;
this.sieveProbe = sieveProbe;
}
Copy link
Member

Choose a reason for hiding this comment

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

That is unfortunate: we tend to put static methods first, then fields, then the constructor, then the methods.

You also moved by mistake in a similar way print and printUsage below.

Could you restore the original ordering of these methods?

Copy link
Author

Choose a reason for hiding this comment

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

I did a Rearrange Code in Intellij which i thought used the checkstyle to rearrange the code accordingly. Reverting it :)

@chibenwa
Copy link
Member

I added the ability to load from a file - but I have to say that I am quite confused what the difference between SieveRepositoryManagement and SieveProbeImpl should be. Looks like quite the duplicate functionality.

Very true.

Note that probes and Management APIs needs to exist: they serve different functionalities:

  • Management and MBean is for some JMX semantic. We do not want it to leak further in the project
  • Probes are a way to interact with a James server. We do rely on them in some tests.

However many of them have duplicate capabilities. Maybe we could have the management stuff calling the probe directly?

@trantienduchn
Copy link

no build result, test this please

@chibenwa
Copy link
Member

We recently did some modifications to the merging tool that runs before the pull request build.

In order to benefit from it, and start the tests, you need to rebase on linagora/master then force push your changes here.

Can I let you do it?

Many thanks!

@matzepan matzepan force-pushed the feature/JAMES-2150-cli-sieve-upload branch from d065070 to c5c56cc Compare October 26, 2018 18:41
@matzepan
Copy link
Author

Rebase is complete.

@chibenwa
Copy link
Member

Hi,

This pull request has just been merged.

Thank you again for your dedication toward the James project. If you need help, want to contribute documentation, new features, tests or bugfix, please don't hesitate to reach us.

Thanks again,

Cheers,

Benoit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution Contribution of a kind contributor - also applicable for community oriented efforts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants