Skip to content

deploy: Adds support for other container engines (ie. podman)#56

Merged
tiny-dancer merged 1 commit intoOptum:mainfrom
ngharo:podman
Sep 9, 2022
Merged

deploy: Adds support for other container engines (ie. podman)#56
tiny-dancer merged 1 commit intoOptum:mainfrom
ngharo:podman

Conversation

@ngharo
Copy link
Contributor

@ngharo ngharo commented Aug 29, 2022 •
edited
Loading

Proposed changes

Adds --container-engine command line option.
Can also be set via container_engine config option. Used underscore for possible future env-var friendliness.

The main driver for this change is to add support for podman.

This also changes container image references to fully qualified, pointed at docker.io registry where runiac images are published.

Issues for these changes

Users may have unexpected results if they're using any container engine other than default docker.

Types of changes

  • Changed any container image references to fully qualified, pointed at docker.io registry where runiac images are published.
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (changes to code, which do not change application behavior)

Further comments

Tested using podman version 3.0.1

@github-actions
Copy link

github-actions bot commented Aug 29, 2022 •
edited
Loading

Unit Test Results

54 tests   54 ✔️  0s ⏱️
15 suites    0 💤
  1 files      0 ❌

Results for commit 4795e24.

♻️ This comment has been updated with latest results.

tiny-dancer
tiny-dancer suggested changes Aug 29, 2022
View reviewed changes
Copy link
Contributor

@tiny-dancer tiny-dancer left a comment

Choose a reason for hiding this comment

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

Let's add 'container engine' support to the config file as well.

@ngharo
Copy link
Contributor Author

ngharo commented Aug 29, 2022

I was wanting that too. I need to dig more into how Viper and Cobra are working together.

I should be able to figure it out quickly and update this PR.

@tiny-dancer
Copy link
Contributor

tiny-dancer commented Aug 31, 2022

@ngharo check out func getDockerfileForBuild() string {

In cmd/cli/deploy for a reference. There may be a better overall connection between the two but that's the existing precedent example for config not passed into the runiac container

(On mobile app sorry for formatting and lack of deep link)

@ngharo ngharo force-pushed the podman branch 2 times, most recently from 94f0e54 to 136fde4 Compare September 1, 2022 01:50
@ngharo
Copy link
Contributor Author

ngharo commented Sep 1, 2022

Thanks for the pointer. Way easier than I thought.

tiny-dancer
tiny-dancer suggested changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@tiny-dancer tiny-dancer left a comment

Choose a reason for hiding this comment

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

Looks good, one minor requested change and then an optional change to consolidate the multiple function calls to one top level function variable.

@ngharo
Copy link
Contributor Author

ngharo commented Sep 1, 2022

What do you think of this approach?

The exported variable ContainerEngine gets set to its default value where it's declared.
The var may be updated by passing the command line arg.
If no cmd line arg, check the config file.

I like this because when I first started looking at this code I was wondering: Why aren't Cobra default value arguments being set in flags and instead defaults asserted via a function call. I get it now that it's for setting command line precedence but it was still little confusing to me.

@tiny-dancer
Copy link
Contributor

tiny-dancer commented Sep 1, 2022

@ngharo looks great, tidy up the tests failing and we’ll be good to go.

@ngharo
Copy link
Contributor Author

ngharo commented Sep 1, 2022 •
edited
Loading

Yeah, about that 🤣

Since it's using Cobra to know if the command option is set, I tried mocking the command line. I can't get viper.Set() in tests to be seen in viper.GetString() in deploy.go. edit: actually was cobra.Flags().Changed() state test challenge.

Pushed up WIP commit. Will revisit this soon.

@ngharo
Adds support for other container engines (ie. podman)
4795e24 
Can be set at the command line via `--container-engine` or
in runiac.yml as `container_engine`.

Normalize on an approach for option precendence
@ngharo
Copy link
Contributor Author

ngharo commented Sep 2, 2022

It was non-trivial to reset cobra.Flags().Changed() state in tests. A simple solution was to change the order of the tests, so admittedly, it's a little cheesed. But this only matters if testing things using cobra.Flags().Changed() logic, which I can't imagine will come up for anything but option precedence tests, which is covered here.

@ngharo ngharo requested a review from tiny-dancer September 9, 2022 01:28
@tiny-dancer
Copy link
Contributor

tiny-dancer commented Sep 9, 2022

Thanks for the bump, LGTM

@tiny-dancer tiny-dancer merged commit 2fe370a into Optum:main Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tiny-dancer tiny-dancer tiny-dancer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ngharo @tiny-dancer