Fixes #24894 - Support for cron expressions #584

sjha4 · 2018-09-11T16:38:57Z

Adds support for custom cron expressions in sync plans.

Requires Katello/katello#7644 , Katello/katello#7700

theforeman-bot · 2018-09-11T16:39:04Z

sjha4 · 2018-09-11T16:52:30Z

@akofink : What does the label Requires katello PR you added mean? Also, I think sync-plans is not covered well in general in hammer..I will try to add some tests around that with this PR..

akofink

I think we need more Katello changes.

akofink · 2018-09-11T16:48:16Z

lib/hammer_cli_katello/sync_plan.rb

@@ -9,6 +9,7 @@ class ListCommand < HammerCLIKatello::ListCommand
        field :sync_date, _("Start Date"), Fields::Date
        field :interval, _("Interval")
        field :enabled, _("Enabled"), Fields::Boolean
+        field :cron_expression, _("Cron Expression")


There's no validation in Katello for this. i.e. require interval to be 'custom cron'

Right..So my intent is to display the cron expression..Should we not do that?

This hammer change is correct, but I feel we should add validation in Katello for this field, as it's only valid when interval == 'custom cron'

akofink · 2018-09-11T16:52:20Z

lib/hammer_cli_katello/sync_plan.rb

@@ -32,9 +33,11 @@ class InfoCommand < HammerCLIKatello::InfoCommand
    class CreateCommand < HammerCLIKatello::CreateCommand
      option "--interval", "INTERVAL", _("how often synchronization should run"),
             :format => HammerCLI::Options::Normalizers::Enum.new(
-               %w(hourly daily weekly)
+               ["hourly", "daily", "weekly", "custom cron"]


Since we need added validation for cron_expression, I think we can also remove this Enum format completely and validate the interval in Katello. Right now, there is validation, but it's not clear what the options are from the error message (note, I removed this format to achieve the following output):

$ hammer sync-plan create --name foo3 --organization-id 1 --interval weeklys --sync-date 2018-09-11 --enabled 1 Could not create the sync plan: Interval not set correctly

I will go ahead and remove the enum format.. Are you suggesting a change in validation message somewhere to prompt the hammer user of valid options?

Not only hammer users, but API users in general. Almost all of our error messages are generated by the API; this is a special case for some reason, and I don't think it needs to be.

Can we change the name of 'custom cron' to 'custom-cron'.
Reasons :

Autocomplete is hammer shell show 'custom cron', if you see hammer shell whenever there are two names are joined using '-' e.g. 'sync-plan', 'organization-id'
below example explains more too

hammer> sync-plan create --name 'test1' --interval custom cron --organization-id 1 --enabled 1 --sync-date '2016-10-01 01:00:00' Could not create the sync plan: Error: Option '--interval': Value must be one of 'hourly', 'daily', 'weekly', 'custom cron'.. See: ' sync-plan create --help'. hammer> sync-plan create --name 'test1' --interval daily --organization-id 1 --enabled 1 --sync-date '2016-10-01 01:00:00'Sync plan created.

As per above example the for daily get autocomplete no need to user to convert it to 'daily', same we can do custom-cron instread of 'custom cron'

@akofink , @sjha4 Let me know what you think ?

I don't think we should change it. Hyphens are used for subcommands and options (i.e. --organization-id), but not for values (i.e. 'Default Organization'). Values with spaces may use quotes or escape the space (i.e. 'Default Organization' or Default\ Organization). This is common throughout the different hammer resources.

Ok, make sense

akofink · 2018-09-11T16:54:52Z

@sjha4 The label means that this hammer PR requires the Katello PR I put in the description. It's mostly helpful for testing and packaging.

akofink · 2018-09-11T16:56:12Z

You will need to update the apipie cache to test this new cron_expression option. Steps 3 through 5 of https://github.com/theforeman/hammer-cli-foreman/tree/master/test/data.

sjha4 · 2018-09-17T15:46:32Z

@akofink : Wondering if the api cache merged 4 days ago for 3.9 would be enough for this?

akofink · 2018-09-17T16:06:27Z

I think it's sufficient.

omkarkhatavkar · 2018-09-18T12:10:29Z

Failed Scenarios:

Failed (should have updated the cron expression with interval)

hammer> sync-plan update --name 'test2' --interval 'custom cron' --organization-id 1 --enabled 0 --sync-date '2016-10-01 01:00:00' --cron-expression "* * 3 * * "
Could not update the sync plan:
  Error: Option '--interval': Value must be one of 'hourly', 'daily', 'weekly'..
  
  See: ' sync-plan update --help'.

Below example fails to exist in previous intervals, maybe we can raise a bug in Redmine. If help suggest that if sync plan date is blank, it should have taken the system date does not seem to be working

hammer> sync-plan create --help
--sync-date SYNC_DATE                   
Start date and time for the sync plan.Time is optional, if kept blank current system time will be considered

hammer> sync-plan create --name 'test1011' --interval daily --organization-id 1 --enabled 1 --sync-date
Could not create the sync plan:
  Error: Option '--sync-date': '' is not a valid date..
  
  See: ' sync-plan create --help'.
hammer> sync-plan create --name 'test1011' --interval daily --organization-id 1 --enabled 1 --sync-date ''
Could not create the sync plan:
  Error: Option '--sync-date': '' is not a valid date..
  
  See: ' sync-plan create --help'.
hammer> sync-plan create --name 'test1011' --interval daily --organization-id 1 --enabled 1 --sync-date ""
Could not create the sync plan:
  Error: Option '--sync-date': '' is not a valid date..
  
  See: ' sync-plan create --help'.
hammer>

3 .Failed should give an error message as weekly interval should not have cron expression (looks like should be fixed by this PR Katello/katello#7700)

hammer> sync-plan create --name "Testing with invalid data 4" --interval weekly --enabled true --organization-id 1 --sync-date "2016-10-01 01:00:00" --cron-expression "* 2 * * *"
Sync plan created.

sjha4 · 2018-09-18T13:58:18Z

@omkarkhatavkar : Ready to retest..

akofink

I can still create invalid sync plans:

$ hammer sync-plan create --name foo3 --organization-id 1 --interval weekly --sync-date 2018-09-11 --enabled 1 --cron-expression "foo"
Sync plan created.

Add validations to prevent --cron-expression when interval != 'custom cron'

omkarkhatavkar · 2018-09-19T08:14:31Z

Scenario_1 got passed now

hammer> sync-plan update --name test --interval 'custom cron' --enabled yes --sync-date "2016-10-01 01:00:00" --organization-id 1 --cron-expression '* * 5 * *'
Sync plan updated.
hammer> sync-plan update --name test --interval 'custom cron' --enabled yes --sync-date "2016-10-01 01:00:00" --organization-id 1 --cron-expression '* * 4 * *'
Sync plan updated.

as @akofink said, yes the sceanario_3 (validation) is still failing.

sjha4 · 2018-09-19T14:42:22Z

@akofink, @omkarkhatavkar : Scenario 3 should give an error now that Katello/katello#7700 is merged..

akofink · 2018-09-19T15:04:12Z

Still erroring on scenario 3 (and yes, I tested this with Katello/katello#7700 checked out before it was merged):

$ hammer sync-plan create --name foo4 --organization-id 1 --interval weekly --sync-date 2018-09-11 --enabled 1 --cron-expression "foo"
Sync plan created.

sjha4 · 2018-09-19T18:35:55Z

Maybe @omkarkhatavkar can confirm this behavior..I spun up a new hammer-devel box and pulled my changes..Works for me on a fresh setup.. 😕

omkarkhatavkar · 2018-09-20T05:21:47Z

👍 @sjha4 @akofink
Scenario 3 is getting passed now

hammer> sync-plan create --name foo4 --organization-id 1 --interval weekly --sync-date 2018-09-11 --enabled 1 --cron-expression "foo"
Could not create the sync plan:
  Validation failed: Custom cron expression only needs to be set for interval value of custom cron

hammer> sync-plan create --name foo4 --organization-id 1 --interval weekly --sync-date 2018-09-11 --enabled 1 --cron-expression "* * * 5 * "
Could not create the sync plan:
  Validation failed: Custom cron expression only needs to be set for interval value of custom cron

hammer> sync-plan create --name foo4 --organization-id 1 --interval daily --sync-date 2018-09-11 --enabled 1 --cron-expression "* * * 5 * "
Could not create the sync plan:
  Validation failed: Custom cron expression only needs to be set for interval value of custom cron

hammer> sync-plan create --name foo4 --organization-id 1 --interval hourly --sync-date 2018-09-11 --enabled 1 --cron-expression "* * * 5 * "
Could not create the sync plan:
  Validation failed: Custom cron expression only needs to be set for interval value of custom cron
hammer>

Scenario 2 is pending now, I'm thinking it actually not related to this PR if we want we can raise separate issue/bug for that. please let me know the thoughts.

akofink · 2018-09-20T12:04:43Z

I figured out why mine wasn't working! Once you generate an apipie cache on your Foreman instance, it uses that for all apidoc requests, even with hammer -r apparently (navigating to https://centos7-devel/apidoc/v2/sync_plans/create.html showed the missing cron_expression param). Regenerating the cache fixed scenario 3 for me. :)

@sjha4 You should add a test or two for this (there are no sync_plan create tests currently).

akofink · 2018-09-20T12:05:58Z

As for scenario 2, let's file a new issue and address it in a separate PR.

sjha4 · 2018-09-20T17:02:03Z

@akofink: Added some tests..

sjha4 · 2018-09-20T17:04:00Z

Forgot about rubocop..

akofink · 2018-09-20T17:13:57Z

test/functional/sync_plan/create_test.rb

+    include SyncPlanHelpers
+
+    it 'create with organization ID,name,interval,date and enabled' do
+      api_expects(:sync_plans, :create, 'create a sync plan') do |par|


@sjha4 sorry, should have mentioned, there's a new syntax (.with_params) which makes the test failures much easier to understand. i.e.

hammer-cli-katello/test/functional/host/extensions/create_test.rb

Line 26 in 49b48b3

with_params('name' => env_name,

akofink · 2018-09-20T17:14:44Z

test/functional/sync_plan/delete_test.rb

@@ -0,0 +1,53 @@
+require_relative '../test_helper'


You don't need to add tests here. Just for create, since that's what this PR is about.

akofink · 2018-09-20T17:14:59Z

test/functional/sync_plan/update_test.rb

@@ -0,0 +1,45 @@
+require_relative '../test_helper'


Same here. You're off the hook for update.

akofink · 2018-09-20T17:16:17Z

test/functional/sync_plan/create_test.rb

@@ -0,0 +1,61 @@
+require_relative '../test_helper'
+require 'hammer_cli_katello/sync_plan'
+require_relative 'sync_plan_helpers'


You're not using this helper in any of the create tests.

sjha4 · 2018-09-20T18:45:51Z

@akofink : Updated the tests to use the with_params..I can remove the update and delete tests if you have any concerns there..

akofink

Tests look good. Thanks @sjha4! Great work! And thanks for the free update/delete tests. :)

akofink · 2018-09-20T18:59:14Z

test/functional/sync_plan/update_test.rb

+  let(:org_name) { "org1" }
+  let(:desc) { "New Description" }
+
+  it 'with organization id and sync plan ID' do


This is a bit weird, but I'll accept it. :) The --organization-id isn't really useful with --id. It's only used to look up the ID using the sync plan name, but I guess it could make sense to test that organization ID isn't being sent to the API, which is one thing this test does.

(cherry picked from commit 3ca2dda)

theforeman-bot added Needs testing Not yet reviewed labels Sep 11, 2018

akofink added the Requires Katello PR label Sep 11, 2018

akofink reviewed Sep 11, 2018

View reviewed changes

sjha4 force-pushed the 24894 branch from aac2cd6 to 4834de1 Compare September 18, 2018 13:44

akofink reviewed Sep 18, 2018

View reviewed changes

sjha4 force-pushed the 24894 branch from 4834de1 to 8126cf0 Compare September 19, 2018 16:43

sjha4 force-pushed the 24894 branch from 8126cf0 to 7e3e5b0 Compare September 20, 2018 17:01

akofink reviewed Sep 20, 2018

View reviewed changes

sjha4 force-pushed the 24894 branch from 7e3e5b0 to a9dd487 Compare September 20, 2018 17:20

Fixes #24894 - Support for cron expressions

89e8101

sjha4 force-pushed the 24894 branch from a9dd487 to 89e8101 Compare September 20, 2018 18:41

akofink approved these changes Sep 20, 2018

View reviewed changes

akofink merged commit 3ca2dda into Katello:master Sep 21, 2018

zjhuntin pushed a commit to zjhuntin/hammer-cli-katello that referenced this pull request Oct 30, 2018

Fixes #24894 - Support for cron expressions (Katello#584)

5e61cbb

(cherry picked from commit 3ca2dda)

Fixes #24894 - Support for cron expressions #584

Fixes #24894 - Support for cron expressions #584

Conversation

sjha4 commented Sep 11, 2018 • edited

theforeman-bot commented Sep 11, 2018

sjha4 commented Sep 11, 2018

akofink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akofink commented Sep 11, 2018

akofink commented Sep 11, 2018

sjha4 commented Sep 17, 2018

akofink commented Sep 17, 2018

omkarkhatavkar commented Sep 18, 2018 • edited

sjha4 commented Sep 18, 2018

akofink left a comment

Choose a reason for hiding this comment

omkarkhatavkar commented Sep 19, 2018

sjha4 commented Sep 19, 2018

akofink commented Sep 19, 2018

sjha4 commented Sep 19, 2018

omkarkhatavkar commented Sep 20, 2018 • edited

akofink commented Sep 20, 2018

akofink commented Sep 20, 2018

sjha4 commented Sep 20, 2018

sjha4 commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjha4 commented Sep 20, 2018

akofink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjha4 commented Sep 11, 2018 •

edited

omkarkhatavkar commented Sep 18, 2018 •

edited

omkarkhatavkar commented Sep 20, 2018 •

edited