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

Extract UI related config params into a UIConfig type #76

Merged
merged 2 commits into from
Aug 11, 2021

Conversation

saurabhnanda
Copy link
Owner

@saurabhnanda saurabhnanda commented Jul 9, 2021

Problem

  1. Config{..} type was mixing up config params required for the job-runner along with the config params required for the web-UI.
  2. There are cases where we want to start job-runners independently of the web-ui (typically you will only have one instance of the web-ui, whereas you may have multiple instances of the job-runner on different machines)
  3. The mixing-up of these two concerns in the Config{..} type forces one to define unnecessary config params if only the web-ui needs to be started or only the job-runner needs to be started

Sub-problem

  • The callback-within-callback style of OddJobs.Cli is very confusing. It was confusing even for me when I wanted to write a custom CLI for some purpose.

Solution

  1. Introduced Types.UIConfig{..}
  2. Introduced Cli.CliType that allows one to easily start only the job-runner, only the web-ui, or both.
  3. OddJobs.Cli parses the command-line and builds a Config -> Config (or UIConfig -> UIConfig) function that overrides the default config based on command-line arguments. This function can be passed to mkConfig or mkUIConfig. This avoid the "callback-withing-callback" style to some extent.

…ob-runner

- Separated Config and UIConfig
- Allowing the Cli to run only the job-runnner, only the web-ui, or both
@saurabhnanda saurabhnanda added the enhancement New feature or request label Jul 9, 2021
@saurabhnanda
Copy link
Owner Author

saurabhnanda commented Jul 9, 2021

@tfausak @kanagarajkm @jappeace @ivb-supercede @leomayleomay @mrputty @aschmois this is a moderately large breaking change. Please share your thoughts about this.

@tfausak
Copy link
Contributor

tfausak commented Jul 9, 2021

I haven't looked at the changes too closely, but it all sounds good to me and we should be able to adapt to these changes.

@kanagarajkm
Copy link
Contributor

@saurabhnanda we don't use the webui and don't fiddle with any config. changes should be fine for us.

@ivb-supercede
Copy link
Collaborator

Sounds good to me.

@saurabhnanda saurabhnanda merged commit 8458b85 into master Aug 11, 2021
@saurabhnanda saurabhnanda deleted the ui-config branch August 11, 2021 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants