lightning icon indicating copy to clipboard operation
lightning copied to clipboard

Persist run options & set at creation (not claim)

Open taylordowns2000 opened this issue 1 year ago • 4 comments

Requesting guidance before going further. Does this feel like the right way to address the run options issue?

Required context (pre-reading) to review:

  • https://github.com/OpenFn/lightning/issues/2079
  • https://github.com/OpenFn/lightning/issues/2082

Note that if we like this idea, next we still need to:

  1. remove the "get options" bit that happens during claim
  2. ensure that these options are delivered properly via the channel to the worker
  3. check to see which options are set by the project admin (e.g., enable dataclip storage) and which options are set by the instance administrator (e.g., max run duration)

taylordowns2000 avatar May 14 '24 14:05 taylordowns2000

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.61%. Comparing base (4af6f2a) to head (b3e7d2c).

:exclamation: Current head b3e7d2c differs from pull request most recent head 3dd2a03

Please upload reports for the commit 3dd2a03 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2085   +/-   ##
=======================================
  Coverage   89.60%   89.61%           
=======================================
  Files         269      270    +1     
  Lines        8949     8955    +6     
=======================================
+ Hits         8019     8025    +6     
  Misses        930      930           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 14 '24 14:05 codecov[bot]

@stuartc , please take a look here! context in the PR itself

taylordowns2000 avatar May 17 '24 06:05 taylordowns2000

@taylordowns2000 this looks like a valid way to implement this. I am not sure I can't validate it's relevancy but it seems to me as a better way of dynamically calculating the run options. Maybe wait for @stuartc to confirm that point.

elias-ba avatar May 23 '24 09:05 elias-ba

@taylordowns2000 this implementation looks good to me (assuming that tests and other bits will come later)

Oh and yes, @taylordowns2000 - tests please!

stuartc avatar May 23 '24 10:05 stuartc

nice. think we're good now @stuartc

taylordowns2000 avatar May 30 '24 09:05 taylordowns2000