Feat: allow passing cronjob specific settings #12

maokomioko · 2025-04-01T04:45:30Z

This originated, because we needed to control these options:

   concurrencyPolicy: Forbid
   successfulJobsHistoryLimit: 1
   failedJobsHistoryLimit: 3

Specifically the concurrency policy to avoid export or import jobs to be run in parallel (when new jobs kicks in before the old one is finished).

maokomioko · 2025-04-01T04:46:47Z

gemfiles/kubeclient_4.gemfile

 source "https://rubygems.org"

-gem "kubeclient", "4.0.0"
+gem "kubeclient", "4.12.0"


Bumped to latest due to vulnerability note:
https://github.com/ManageIQ/kubeclient?tab=readme-ov-file#vulnerability-in--v492

sdhull

Looking at this PR, it seems like there are a ton of aesthetic/rubocop-y type changes that are insubstantial as far as the primary aim of the PR goes.

PRs are much easier to review and discuss if you separate aesthetic changes into a different PR that has no functional changes. That way contributors can discuss aesthetics/style without holding up valuable functional changes, which I think this PR probably has.

Also, I'd love to see new test cases that prove the new functionality that was added works as intended

sdhull · 2025-09-03T23:08:31Z

Gemfile


 gemspec
+
+group :development, :test do


Is there some reason you moved these out of the gemspec? I seem to recall that keeping dev dependencies in the gemspec is more standard / better practice

sdhull · 2025-09-03T23:09:19Z

cron-kubernetes.gemspec

  spec.homepage      = "https://github.com/keylimetoolbox/cron-kubernetes"
  spec.license       = "MIT"
-  spec.required_ruby_version = ">= 3.2"
+  spec.required_ruby_version = "~> 3.4"


Unless it's absolutely required to accomplish the goals in this PR, I'd recommend separating out ruby version changes

sdhull · 2025-09-03T23:09:53Z

spec/spec_helper.rb

 # Requires supporting ruby files with custom matchers and macros, etc,
 # in spec/support/ and its subdirectories.
-Dir[File.expand_path("support/**/*.rb", __dir__)].sort.each { |f| require f }
+Dir[File.expand_path("support/**/*.rb", __dir__)].each { |f| require f }


Sorting files in the support directory is fairly standard practice and can help manage order dependencies in these files. Why was this removed?

sdhull · 2025-09-03T23:10:41Z

spec/cron_kubernetes/scheduler_spec.rb

    context "when RAILS_ENV is defined" do
      before do
-        @rails_env = ENV["RAILS_ENV"]
+        @rails_env = ENV.fetch("RAILS_ENV", nil)


Changing from bracket to fetch("KEY", nil) seems unnecessary and additional characters for no additional benefit. What's the reasoning here?

jeremywadsack

@maokomioko Thanks for the submission. Sorry for the delay in responding.

Some comments on the code below.

Overall, I'm open to adding more configurability for the cron job. I understand the need for that. There seems to be a number of changes here that are out of scope of that update as reflected in my comments and sdhull's.

Please add test cases that exercise the cron_job_settings code and confirm it works as expected.

As you have added a new parameter to the public interface for the gem, please update the README needs to be update to reflect that and include examples for why you would use that.

jeremywadsack · 2025-09-28T23:13:36Z

lib/cron_kubernetes/context/kubectl.rb


      def google_application_default_credentials
-        return unless defined?(Google) && defined?(Google::Auth)
+        return unless defined?(Google::Auth)


With your change, it seems this could fail if you don't have Google defined in your environment. I'm not sure why you altered this.

jeremywadsack · 2025-09-28T23:14:27Z

lib/cron_kubernetes/cron_job.rb

-      @identifier   = identifier
+    attr_accessor :schedule, :command, :job_manifest, :name, :identifier, :cron_job_settings
+
+    def initialize(options = {})


I realize that Rubocop doesn't like lots of options on method names, but this makes the method a lot less self-documenting.

jeremywadsack · 2025-09-28T23:16:10Z

lib/cron_kubernetes/cron_job.rb

+    def build_cron_job_spec
+      default_spec = {
+          "schedule" => schedule,
+          "concurrencyPolicy" => "Forbid",


Hard-coding this into the spec seems is a bad idea, especially as the default value is "allow". This change would lead to unexpected behavior for those expecting the default behavior out of the box.

jeremywadsack · 2025-09-28T23:18:09Z

lib/cron_kubernetes/scheduler.rb


    def rails_env
-      ENV["RAILS_ENV"]
+      ENV.fetch("RAILS_ENV", nil)


I don't understand how this behavior is different than the code that was there.

jeremywadsack · 2025-09-28T23:20:17Z

spec/cron_kubernetes/kubeclient_context_spec.rb

+# Define Struct classes to replace OpenStruct
+KubeConfigContext = Struct.new(:api_endpoint, :api_version, :namespace, :auth_options, :ssl_options)
+KubeConfig = Struct.new(:context)


Thank you for making this update

jeremywadsack · 2025-09-28T23:23:36Z

.rubocop.yml

+  EnforcedHashRocketStyle: key 
+  EnforcedColonStyle: key 


I personally find table layout more readable than key. In any case, I'm not sure why you are suggesting a change to the style of this gem.

maokomioko · 2025-09-28T23:28:28Z

Hi guys,
Sure - let me get back to you with revised version. I'll keep changes to the minimum, removing the flavors.

maokomioko added 2 commits April 1, 2025 11:41

Allow passing cronjob options

e0b07dd

Rubocop EnforcedHashRocketStyle -> key

566c93a

maokomioko commented Apr 1, 2025

View reviewed changes

sdhull reviewed Sep 3, 2025

View reviewed changes

jeremywadsack requested changes Sep 28, 2025

View reviewed changes

Feat: allow passing cronjob specific settings #12

Are you sure you want to change the base?

Feat: allow passing cronjob specific settings #12

Uh oh!

Conversation

maokomioko commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdhull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremywadsack left a comment

Choose a reason for hiding this comment

Uh oh!

jeremywadsack Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremywadsack Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremywadsack Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maokomioko commented Sep 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeremywadsack Sep 28, 2025 •

edited

Loading

jeremywadsack Sep 28, 2025 •

edited

Loading

jeremywadsack Sep 28, 2025 •

edited

Loading