adds `migrate` and `run ansible` commands #887

mhrivnak · 2019-01-07T20:27:28Z

The operator-sdk migrate command adds go source and other files necessary to
convert an ansible operator to a hybrid operator.

The scaffolds used for migrate are also made available for use in creating
the ansible-operator base image through
commands/ansible-operator-base/main.go.

The operator-sdk run ansible command uses the operator-sdk binary as the
production-use ansible operator binary.

fixes #886

The `operator-sdk migrate` command adds go source and other files necessary to convert an ansible operator to a hybrid operator. The scaffolds used for `migrate` are also made available for use in creating the ansible-operator base image through `commands/ansible-operator-base/main.go`. The `operator-sdk run ansible` command uses the operator-sdk binary as the production-use ansible operator binary. fixes operator-framework#886

commands/operator-sdk/cmd/migrate.go

commands/operator-sdk/cmd/run/ansible.go

hack/tests/e2e-ansible.sh

pkg/scaffold/ansible/gopkgtoml.go

pkg/scaffold/ansible/main.go

estroz · 2019-01-07T23:34:02Z

@mhrivnak why do we need both commands/ansible-operator-base/main.go and operator-sdk migrate? Can the former not be a part of the latter? Perhaps I'm missing something.

mhrivnak · 2019-01-08T16:39:59Z

@mhrivnak why do we need both commands/ansible-operator-base/main.go and operator-sdk migrate? Can the former not be a part of the latter? Perhaps I'm missing something.

The migrate command is for an end user who already has a functioning ansible operator and wants to turn that into a hybrid operator.

The commands/ansible-operator-base/main.go file is for our own release engineering only, to create the ansible-operator base container image starting from zero. Differences from the migrate command include: there is no Go source code involved, no Gopkg.toml, no ansible role or playbook, no CRD, no watches.yaml file, no existing Dockerfile that needs to be replaced. As opposed to the migrate command, which lays down a main.go that will eventually be compiled into the operator binary, this base image creation workflow assumes that the operator-sdk binary itself will be built through other means and added to the container image that's being composed.

FWIW I don't know if this is the best location for this file. Since it is for release engineering only, maybe it would be more clear to move it into hack/image/ or similar?

estroz · 2019-01-09T00:16:35Z

@mhrivnak thanks for clarifying. I agree, since commands/ansible-operator-base/main.go isn't intended to be a compiled command it should go into hack.

shawn-hurley

Some user experience questions

shawn-hurley · 2019-01-09T14:52:15Z

commands/operator-sdk/cmd/migrate.go

+		Watches: true,
+		Roles:   true,
+	}
+	_, err := os.Stat("playbook.yaml")


Wonder if this should be a flag or at least alert the user that we are assuming no playbook if this is not found. I could see cases, where playbooks are in a new directory and users, have changed things. Telling the users what we are doing, and what they can do to change it back if that is incorrect might be a good idea. Thoughts?

Yeah a good example of this would be the etcd operator which puts all of it's ansible code under a folder ansible.

Sounds good. If they put it in a different location that what we expect, they'll have to manually add it to their new Dockerfile. Presumably they'd catch that in comparing their old Dockerfile to new, but it's worth calling out explicitly.

shawn-hurley · 2019-01-09T16:16:01Z

commands/operator-sdk/cmd/migrate.go

+	}
+
+	dockerfilePath := filepath.Join(scaffold.BuildDir, scaffold.DockerfileFile)
+	err = os.Rename(dockerfilePath, dockerfilePath+".sdkold")


I think we need to alert the user that we took this action after this finishes IMO.

Also, at the end of the command, should we run dep ensure? or should we ask the user to run dep ensure?

Agreed to the alerting. I'll log something about it.

I lean toward not running dep automatically, in part because it will cause the e2e tests to fail in cases like we see here where operator-sdk has changes that aren't in the upstream repo. It makes me think there are enough other cases where the user doesn't want to run dep ensure right away, that we shouldn't do it automatically.

dymurray

Like the approach you took and I had success with the migrate and run commands. Good stuff!

release.sh

fixes version check in `release.sh` Co-Authored-By: mhrivnak <mhrivnak@hrivnak.org>

mhrivnak · 2019-01-09T19:17:01Z

@estroz thanks for the assist with release.sh! I think it should be set now.

hasbro17 · 2019-01-09T23:22:32Z

@mhrivnak Can you please update the CHANGELOG for the migrate and run ansible commands as well?

hasbro17 · 2019-01-09T23:35:22Z

We also need to update the CLI reference guide for the new commands.
https://github.com/operator-framework/operator-sdk/blob/master/doc/sdk-cli-reference.md

hasbro17 · 2019-01-10T00:18:44Z

Also the relevant issue for this PR is #860, not #886 I think.

mhrivnak · 2019-01-10T14:13:30Z

@mhrivnak Can you please update the CHANGELOG for the migrate and run ansible commands as well?

Yes, I'll add that and a bit of documentation separately.

mhrivnak added kind/feature Categorizes issue or PR as related to a new feature. language/ansible Issue is related to an Ansible operator project labels Jan 7, 2019

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 7, 2019

estroz suggested changes Jan 7, 2019

View reviewed changes

addresses review comments

0e9a69c

mhrivnak mentioned this pull request Jan 8, 2019

adds migrate command and ansible base image scaffolds #823

Closed

shawn-hurley approved these changes Jan 9, 2019

View reviewed changes

dymurray approved these changes Jan 9, 2019

View reviewed changes

estroz reviewed Jan 9, 2019

View reviewed changes

release.sh Outdated Show resolved Hide resolved

estroz reviewed Jan 9, 2019

View reviewed changes

release.sh Outdated Show resolved Hide resolved

estroz and others added 2 commits January 9, 2019 14:07

Apply suggestions from code review

1d004aa

fixes version check in `release.sh` Co-Authored-By: mhrivnak <mhrivnak@hrivnak.org>

Additional improvements based on review.

c845851

update link in docs

96b06a2

estroz approved these changes Jan 9, 2019

View reviewed changes

mhrivnak merged commit dd62eaa into operator-framework:master Jan 9, 2019

mhrivnak deleted the migrate-and-run branch January 9, 2019 22:58

hasbro17 mentioned this pull request Jan 10, 2019

Use operator-sdk binary as operator binary for helm and ansible #860

Closed

joelanford mentioned this pull request Jan 10, 2019

commands,pkg/scaffold,hack,pkg/helm: run and migrate command support for helm #897

Merged

hasbro17 mentioned this pull request Jan 18, 2019

Support hybrid operators #670

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds `migrate` and `run ansible` commands #887

adds `migrate` and `run ansible` commands #887

mhrivnak commented Jan 7, 2019

estroz commented Jan 7, 2019

mhrivnak commented Jan 8, 2019

estroz commented Jan 9, 2019 •

edited

Loading

shawn-hurley left a comment

shawn-hurley Jan 9, 2019

dymurray Jan 9, 2019

mhrivnak Jan 9, 2019

shawn-hurley Jan 9, 2019

shawn-hurley Jan 9, 2019

mhrivnak Jan 9, 2019

mhrivnak Jan 9, 2019

dymurray left a comment

mhrivnak commented Jan 9, 2019

hasbro17 commented Jan 9, 2019

hasbro17 commented Jan 9, 2019

hasbro17 commented Jan 10, 2019

mhrivnak commented Jan 10, 2019

adds migrate and run ansible commands #887

adds migrate and run ansible commands #887

Conversation

mhrivnak commented Jan 7, 2019

estroz commented Jan 7, 2019

mhrivnak commented Jan 8, 2019

estroz commented Jan 9, 2019 • edited Loading

shawn-hurley left a comment

Choose a reason for hiding this comment

shawn-hurley Jan 9, 2019

Choose a reason for hiding this comment

dymurray Jan 9, 2019

Choose a reason for hiding this comment

mhrivnak Jan 9, 2019

Choose a reason for hiding this comment

shawn-hurley Jan 9, 2019

Choose a reason for hiding this comment

shawn-hurley Jan 9, 2019

Choose a reason for hiding this comment

mhrivnak Jan 9, 2019

Choose a reason for hiding this comment

mhrivnak Jan 9, 2019

Choose a reason for hiding this comment

dymurray left a comment

Choose a reason for hiding this comment

mhrivnak commented Jan 9, 2019

hasbro17 commented Jan 9, 2019

hasbro17 commented Jan 9, 2019

hasbro17 commented Jan 10, 2019

mhrivnak commented Jan 10, 2019

adds `migrate` and `run ansible` commands #887

adds `migrate` and `run ansible` commands #887

estroz commented Jan 9, 2019 •

edited

Loading