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

Add options to Workflow::Runner initialize #48

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 20, 2023

Add a way for the caller to initialize the docker runner and pass options.

This is going to be important for the kubernetes runner because we're going to have to pass options for authentication

@agrare agrare requested a review from Fryguy as a code owner June 20, 2023 21:28
@agrare agrare added the enhancement New feature or request label Jun 21, 2023
@agrare agrare force-pushed the add_method_for_passing_options_to_docker_runner_klass branch from 4837c49 to d2609ec Compare June 21, 2023 17:47
README.md Outdated Show resolved Hide resolved
exe/floe Outdated Show resolved Hide resolved
@agrare agrare force-pushed the add_method_for_passing_options_to_docker_runner_klass branch from d2609ec to 6130e49 Compare June 21, 2023 18:18
exe/floe Outdated Show resolved Hide resolved
Add a way for the caller to initialize the docker runner and pass
options.
@agrare agrare force-pushed the add_method_for_passing_options_to_docker_runner_klass branch from 6130e49 to 0e83337 Compare June 21, 2023 18:20
exe/floe Outdated Show resolved Hide resolved
exe/floe Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
exe/floe Outdated Show resolved Hide resolved
@agrare agrare force-pushed the add_method_for_passing_options_to_docker_runner_klass branch 2 times, most recently from d0b7e72 to e437bd0 Compare June 21, 2023 18:43
@agrare agrare force-pushed the add_method_for_passing_options_to_docker_runner_klass branch from e437bd0 to 366269d Compare June 21, 2023 18:44
exe/floe Outdated
Floe::Workflow::Runner::Kubernetes
end

runner_options = opts[:docker_runner_options].to_h { |opt| opt.split("=") }
Copy link
Member

Choose a reason for hiding this comment

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

I believe we expect only the first = to be the delimiter, and any subsequent ones would be part of the value, so I think this is more accurate:

Suggested change
runner_options = opts[:docker_runner_options].to_h { |opt| opt.split("=") }
runner_options = opts[:docker_runner_options].to_h { |opt| opt.split("=", 2) }

Copy link
Member

Choose a reason for hiding this comment

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

Might be useful to have a spec for this case too

Copy link
Member

Choose a reason for hiding this comment

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

@agrare If possible, please add a spec for this case.

Copy link
Member

Choose a reason for hiding this comment

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

Oh nvm, I thought the runner spec was doing a full run using the CLI, but it's only stubbing the underlying kubernetes call and then calling fro the runner class directly. Since we don't have specs for the floe script, I can merge now and we can include that in future specs for that executable.

@miq-bot
Copy link
Member

miq-bot commented Jun 21, 2023

Checked commits agrare/floe@bdf3751~...7982080 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Rubocop - missing config files

@Fryguy Fryguy merged commit 1b5541c into ManageIQ:master Jun 21, 2023
@agrare agrare deleted the add_method_for_passing_options_to_docker_runner_klass branch June 21, 2023 21:49
agrare added a commit that referenced this pull request Jul 6, 2023
Added
- Add ability to pass options to `Floe::Workflow::Runner` (#48)
- Add kubeconfig file support to `Floe::Workflow::Runner::Kubernetes` (#53)

Removed
- Remove to_dot/to_svg code (#54)

Fixed
- Fixed default rake task to spec (#55)
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.

3 participants