Skip to content

[ansible/helm] remove operator-sdk binary from ansible/helm base images#3331

Merged
varshaprasad96 merged 4 commits intooperator-framework:masterfrom
varshaprasad96:modify/entrypoint
Jul 8, 2020
Merged

[ansible/helm] remove operator-sdk binary from ansible/helm base images#3331
varshaprasad96 merged 4 commits intooperator-framework:masterfrom
varshaprasad96:modify/entrypoint

Conversation

@varshaprasad96
Copy link
Copy Markdown
Member

@varshaprasad96 varshaprasad96 commented Jul 1, 2020

Description of the change:

Remove the subcommand operator-sdk exec-entrypoint which is only valid for Ansible/Helm-based Operators.
Add the main.go to run the operators in cmd/<helm|ansible>-operator/main.go
Build cmd/<helm|ansible>-operator/main.go for Ansible/Helm base images.
Note that, the script buid-<ansible|helm>-image.sh builds the binary from cmd/ansible|helm-operator/main.go. The Dockerfile scaffolding now copies this binary instead of SDK's binary. The entrypoint script can now run the generated base-image binary instead of exec-entrypoint command.

Motivation for the change:
Remove exec-entrypoint command and hence also remove operator-sdk binary from ansible/helm base image. Its intention is to provide helm/ansible binaries which can be used to run the operator projects locally in order to replace the operator-sdk run local command. It will be done in follow-ups. E.g:

$ go build -o helm-operator ./cmd/helm-operator/main.go
# will allow we run a helm-operator with
$ helm-operator run 
{"level":"info","ts":1594191796.834888,"logger":"setup","msg":"version information","go":"go1.13.12","GOOS":"darwin","GOARCH":"amd64","helm-operator":"0.0.0+git"}
{"level":"info","ts":1594191796.8350248,"logger":"setup","msg":"watching all namespaces"}
...

Checklist
If the pull request includes user-facing changes, extra documentation is required:

@varshaprasad96 varshaprasad96 requested a review from fabianvf July 1, 2020 22:43
@varshaprasad96 varshaprasad96 force-pushed the modify/entrypoint branch 2 times, most recently from 88eef98 to 242d85a Compare July 2, 2020 00:28
@varshaprasad96 varshaprasad96 changed the title [ansible] generate binary to run ansible operator [ansible/helm] remove operator-sdk binary from ansible/helm base images Jul 2, 2020
Comment thread pkg/helm/image/base_image_build.go Outdated
Comment thread internal/scaffold/helm/entrypoint.go Outdated
Comment thread internal/scaffold/helm/dockerfilehybrid.go Outdated
Comment thread hack/image/helm/baseimage/baseimage.go
Comment thread hack/image/ansible/baseimage/baseimage.go Outdated
Comment thread hack/image/build-helm-image.sh Outdated
Comment thread hack/tests/e2e-ansible.sh Outdated
Comment thread hack/tests/e2e-ansible.sh Outdated
Comment thread cmd/helm-operator/main.go Outdated
Comment thread hack/image/build-ansible-image.sh
Comment thread cmd/ansible-operator/main.go Outdated
Comment thread cmd/ansible-operator/main.go
Comment thread cmd/ansible-operator/main.go Outdated
Comment thread cmd/helm-operator/main.go
@varshaprasad96 varshaprasad96 force-pushed the modify/entrypoint branch 3 times, most recently from 3946f9f to 2e59955 Compare July 6, 2020 01:00
Comment thread cmd/ansible-operator/main.go Outdated
Comment thread cmd/helm-operator/main.go
Comment thread internal/scaffold/helm/entrypoint.go
Comment thread internal/scaffold/helm/dockerfilehybrid.go
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

IMO it still missing some cleanups to avoid the code duplication and shows that it is missing to do the fragment, remove the dir execentrypoint and others actions, see the comments.

Comment thread cmd/ansible-operator/main.go Outdated
Comment thread internal/scaffold/helm/entrypoint.go
This PR intends to create an independent binary using exec-entrypoint
command for ansible operators.
Comment on lines +3 to +4
The `operator-sdk` command no longer supports `exec-entrypoint` subcommand for ansible and helm operators. The entrypoints
for ansible/helm base images are from separate binaries present inside `cmd/` folder.
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Jul 8, 2020

Choose a reason for hiding this comment

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

Regards

The entrypoints for ansible/helm base images are from separate binaries present inside cmd/ folder.

Note that, the fragment is used to generate the CHANGELOG and migration guide and the cmd folder does not exist in the user's projects.

Following a suggestion.

Suggested change
The `operator-sdk` command no longer supports `exec-entrypoint` subcommand for ansible and helm operators. The entrypoints
for ansible/helm base images are from separate binaries present inside `cmd/` folder.
**(Valid just for Ansible/Helm-based Operators).** The subcommand `operator-sdk exec-entrypoint` was removed.

I understand that the end goal of this change is for we provide the Helm/Ansible binaries which will allow us to run the projects. (which would be a follow up to have the makefile targets and etc) Example:

$ go build -o helm-operator ./cmd/helm-operator/main.go
# will allow we run a helm-operator with
$ helm-operator run 
{"level":"info","ts":1594191796.834888,"logger":"setup","msg":"version information","go":"go1.13.12","GOOS":"darwin","GOARCH":"amd64","helm-operator":"0.0.0+git"}
{"level":"info","ts":1594191796.8350248,"logger":"setup","msg":"watching all namespaces"}
{"level":"info","ts":1594191797.146786,"logger":"controller-runtime.metrics","msg":"metrics server is starting to listen","addr":":8080"}
{"level":"error","ts":1594191797.146928,"logger":"setup","msg":"unable to load watches.yaml","path":"./watches.yaml","error":"open ./watches.yaml: no such file or directory"}

Usually, when we remove some command we would provide the info/steps for the users does the same in the new way. However, in this case, I do not think that at this moment would make sense we add info such as:

To runs as Ansible/Helm-based operator from inside of a Pod in a cluster use SDK source code and go run cmd/<ansible|helm>-operator/main.go instead of it.

Because it would be replaced in the future by the binaries.

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Just a few nits

  • See my comment regards the fragment description. https://github.com/operator-framework/operator-sdk/pull/3331/files#r451307372
  • Also, could you please provide a better/more accurate information in the first comment of this PR for we are able to track it? Could you please register the required follow-ups and the end goals (motivation) of this change? Note that when we merge the PR we can add the description field to be the commit description as well which allows us have this info when we check the commit. E.g

Screenshot 2020-07-08 at 08 19 52

Following my suggestion:

Description of the change:

  • Remove the subcommand operator-sdk exec-entrypoint which is just valid for Ansible/Helm-based Operators.
  • Add the main.go to run the operators in cmd/<helm|ansible>-operator/main.go
  • Build cmd/<helm|ansible>-operator/main.go in Ansible/Helm the base dir images

Note that, the script buid-<ansible|helm>-image.sh builds the binary from /hack/image/ansible/baseimage/baseimage.go. The Dockerfile scaffolding is modified to copy this binary. The entrypoint script can now run the generated base-image binary instead of exec-entrypoint command.

Motivation for the change:
Remove exec-entrypoint command and hence also remove operator-sdk binary from ansible/helm base image. Its intention is to provide helm/ansible binaries instead which can be used to run the operators projects locally in order to replace the operator-sdk run local command. It will be done in follow-ups. E.g:

$ go build -o helm-operator ./cmd/helm-operator/main.go
# will allow we run a helm-operator with
$ helm-operator run 
 {"level":"info","ts":1594191796.834888,"logger":"setup","msg":"version information","go":"go1.13.12","GOOS":"darwin","GOARCH":"amd64","helm-operator":"0.0.0+git"}
{"level":"info","ts":1594191796.8350248,"logger":"setup","msg":"watching all namespaces"}
{"level":"info","ts":1594191797.146786,"logger":"controller-runtime.metrics","msg":"metrics server is starting to listen","addr":":8080"}
{"level":"error","ts":1594191797.146928,"logger":"setup","msg":"unable to load watches.yaml","path":"./watches.yaml","error":"open ./watches.yaml: no such file or directory"} 

Then, for me, it is /lgtm /approve after address the above suggestions.

Comment thread changelog/fragments/remove-exec-entrypoint.yaml Outdated
Copy link
Copy Markdown
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM after removing the changelog fragment. (sorry for the back-and-forth!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants