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.

Sign up for GitHub

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

Jump to bottom

Allows OLM subcommands work for Helm/Ansible new layout #3341

Merged
merged 4 commits into from
Jul 27, 2020
Merged

Allows OLM subcommands work for Helm/Ansible new layout #3341

merged 4 commits into from
Jul 27, 2020

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Jul 3, 2020 •
edited
Loading

Description of the change:

  • fix OLM subcommands to be able to get the operator name via the project name in the project file or the current directory.
  • add e2e tests to ensure that the bundle subcommands are working for projects scaffolds with Helm plugin and the new layout.

Motivation

SDK is in a process to be integrated with KB which means that its project layouts will be aligned. More info : Integrating Kubebuilder and Operator SDK.

#3327

@camilamacedo86 camilamacedo86 mentioned this pull request Jul 3, 2020
Closed
@camilamacedo86 camilamacedo86 added language/ansible Issue is related to an Ansible operator project language/helm Issue is related to a Helm operator project kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form labels Jul 3, 2020
@camilamacedo86 camilamacedo86 changed the title fix: OLM commands for Helm/Ansible in the new layout Adapt OLM commands for Helm/Ansible in the new layout Jul 3, 2020
@camilamacedo86 camilamacedo86 changed the title Adapt OLM commands for Helm/Ansible in the new layout WIP : Adapt OLM commands for Helm/Ansible in the new layout Jul 3, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2020
@camilamacedo86 camilamacedo86 changed the title WIP : Adapt OLM commands for Helm/Ansible in the new layout WIP : Adapt OLM subcommand to work with Helm/Ansible scaffold with the new layout (make bundle, make bundle-build, operator-sdk generate kustomize manifests) should work Jul 3, 2020
@camilamacedo86 camilamacedo86 changed the title WIP : Adapt OLM subcommand to work with Helm/Ansible scaffold with the new layout (make bundle, make bundle-build, operator-sdk generate kustomize manifests) should work WIP : Adapt OLM subcommand to work with Helm/Ansible scaffold with the new layout (make bundle, make bundle-build, operator-sdk generate kustomize manifests) Jul 3, 2020
@estroz

This comment has been minimized.

Sign in to view
@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2020
@camilamacedo86

This comment has been minimized.

Sign in to view
@camilamacedo86 camilamacedo86 mentioned this pull request Jul 3, 2020
Merged
@camilamacedo86 camilamacedo86 added blocked and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 3, 2020
@camilamacedo86 camilamacedo86 changed the title WIP : Adapt OLM subcommand to work with Helm/Ansible scaffold with the new layout (make bundle, make bundle-build, operator-sdk generate kustomize manifests) Adapt OLM subcommand to work with Helm/Ansible scaffold with the new layout (make bundle, make bundle-build, operator-sdk generate kustomize manifests) Jul 3, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2020
@camilamacedo86

This comment has been minimized.

Sign in to view
@estroz

This comment has been minimized.

Sign in to view
@camilamacedo86

This comment has been minimized.

Sign in to view
@camilamacedo86 camilamacedo86 mentioned this pull request Jul 8, 2020
Closed
@camilamacedo86 camilamacedo86 force-pushed the fix-olm-for-new branch 3 times, most recently from 0db4250 to 7518ca5 Compare July 25, 2020 10:06
@camilamacedo86 camilamacedo86 mentioned this pull request Jul 25, 2020
Closed
92 tasks
@camilamacedo86 camilamacedo86 mentioned this pull request Jul 26, 2020
Merged
@camilamacedo86 camilamacedo86 force-pushed the fix-olm-for-new branch 2 times, most recently from 3ed251c to 0540cbb Compare July 27, 2020 18:55
estroz
estroz approved these changes Jul 27, 2020
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2020
joelanford
joelanford approved these changes Jul 27, 2020
View reviewed changes
Copy link
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

test/e2e-helm/e2e_helm_olm_test.go
Comment on lines +75 to +99
By("adding the 'packagemanifests' rule to the Makefile")
err = tc.AddPackagemanifestsTarget()
Expect(err).Should(Succeed())

By("generating the operator package manifests")
err = tc.Make("packagemanifests")
Expect(err).NotTo(HaveOccurred())

By("updating clusterserviceversion with the manager image")
testutils.ReplaceInFile(
filepath.Join(tc.Dir, "packagemanifests", operatorVersion,
fmt.Sprintf("e2e-%s.clusterserviceversion.yaml", tc.TestSuffix)),
"controller:latest", tc.ImageName)

By("installing crds to run packagemanifests")
err = tc.Make("install")
Expect(err).NotTo(HaveOccurred())

By("running the package")
runPkgManCmd := exec.Command(tc.BinaryName, "run", "packagemanifests",
"--install-mode", "AllNamespaces",
"--operator-version", operatorVersion,
"--timeout", "4m")
_, err = tc.Run(runPkgManCmd)
Expect(err).NotTo(HaveOccurred())
Copy link
Member

Choose a reason for hiding this comment

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

Can we separate the generate/run bundle tests from the generate/run packagemanifests tests?

Can be done in a follow-up

website/content/en/docs/olm-integration/generation.md
Comment on lines +126 to +145
**For Go-based Operator projects**

```make
# Options for "packagemanifests".
ifneq ($(origin FROM_VERSION), undefined)
PKG_FROM_VERSION := --from-version=$(FROM_VERSION)
endif
ifneq ($(origin CHANNEL), undefined)
PKG_CHANNELS := --channel=$(CHANNEL)
endif
ifeq ($(IS_CHANNEL_DEFAULT), 1)
PKG_IS_DEFAULT_CHANNEL := --default-channel
endif
PKG_MAN_OPTS ?= $(FROM_VERSION) $(PKG_CHANNELS) $(PKG_IS_DEFAULT_CHANNEL)

# Generate package manifests.
packagemanifests: kustomize manifests
operator-sdk generate kustomize manifests -q
$(KUSTOMIZE) build config/manifests | operator-sdk generate packagemanifests -q --version $(VERSION) $(PKG_MAN_OPTS)
```
Copy link
Member

Choose a reason for hiding this comment

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

Ouch - kind of a bummer that we have to duplicate this entire block because of the single manifests dependency on the packagemanifests target.

This is one of those things that makes organization on our docs site extremely difficult when dealing with three operator types.

I'm almost to the point where I could be convinced to duplicate this (or some portion of this) doc (and other docs with similar issues) for each operator type.

This is okay for now, but definitely something we could think about improving later.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that we need to clean up the docs .. keeping it mind for follow-ups. My suggestion here was added just a note for Ansible/Helm since we need just remove the manifests. However, @estroz thought that would be easier understand by duplicating which is true as well/.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@camilamacedo86 camilamacedo86 merged commit 56b50ac into operator-framework:master Jul 27, 2020
@camilamacedo86 camilamacedo86 deleted the fix-olm-for-new branch July 27, 2020 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estroz estroz estroz approved these changes

@joelanford joelanford joelanford approved these changes

@jmccormick2001 jmccormick2001 Awaiting requested review from jmccormick2001

@asmacdo asmacdo Awaiting requested review from asmacdo

@jmrodri jmrodri Awaiting requested review from jmrodri

Assignees

@joelanford joelanford

@estroz estroz

Labels
kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form language/ansible Issue is related to an Ansible operator project language/helm Issue is related to a Helm operator project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@camilamacedo86 @estroz @openshift-ci-robot @joelanford