[olm-cmd] Package SDK with a default version of OLM #3297

varshaprasad96 · 2020-06-26T02:22:21Z

Description of the change:
This PR uses go-bindata to embed the default version (most recent successful release) of
OLM manifests when user does not have any preference, so that SDK need not fetch
the manifests from github everytime.
The OLM manifests in this PR have the version "v0.15.1". With every
release of SDK, olm-bindata-vX.X.X.go needs to be updated
with the most successful released version of OLM manifests.

Motivation for the change:
Packaging the most successful released version of OLM along with SDK,
so that for every install the manifests need not be fetched from GitHub. This
will also help in avoiding errors when the "latest" tagged version of OLM has some
problems after its release.

varshaprasad96 · 2020-06-26T02:36:55Z

@estroz @camilamacedo86 @joelanford
This PR also addresses the issue #3095 . If we intend to follow this approach, these are few other suggestions:

Can we have a script during release process, which updates olm-bindata with the most successfully released version (tagged "latest") of OLM. If we decide to do so, when do we plan to update the default OLM version - only with major releases of SDK, or even minor release (if a new OLM release falls in-between).
Could we include in every release note of SDK the particular version of OLM being included? (ie here)
The release process may also need to be updated to include an additional step that the person performing the release should check whether the "latest" release of OLM was successful. For example, something like 0.15.0 should not be shipped with SDK.

joelanford

Can we include the generation of the bindata file in the normal build process? (e.g. with a new gen-bindata target that the build/* and install targets depend on)?

cmd/operator-sdk/olm/install.go

internal/olm/client.go

internal/olm/manager.go

jmccormick2001

apart from Joe's comments, this tested out ok on my local dev box, seems to work...I guess the changelog fragment for this is all I can think of that might be necessary.

camilamacedo86 · 2020-07-04T08:34:47Z

go.mod

@@ -31,8 +31,10 @@ require (
 	github.com/spf13/viper v1.4.0
 	github.com/stretchr/testify v1.5.1
 	go.uber.org/zap v1.14.1
+	golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect


We do not need usually add the indirect. could you please remove and run go mod tidy to check?

I ran go mod tidy, still they persist.

The go mod tidy sometimes do not remove. You can remove manually and then run go mod tidy to ensure.
You can also run go mod why golang.org/x/lint v0.0.0-20200302205851-738671d3881b. If its output contains (main module does not need package v0.0.0-20200302205851-738671d3881b) it can be removed safely.

This PR uses go-bindata to embed a default version of OLM, when user does not have any preference, so that SDK need not fetch the manifests from github everytime. The OLM manifests in this PR have the version "v0.15.1". With every release SDK the olm-bindata-vX.X.X.go intends to be updated with the most successful released version of OLM manifests.

varshaprasad96 · 2020-07-08T05:19:45Z

@joelanford @camilamacedo86 @estroz Made modifications as suggested and added relevant documentation. Please have a look.

camilamacedo86 · 2020-07-08T07:53:03Z

changelog/fragments/add-deafult-olm-version.yaml

+      modify default OLM version and update manifests accordingly.
+    kind: "addition"
+    # Is this a breaking change?
+    breaking: true


Why it is a breaking change? If yes, then it is missing the steps that should be used in the migration guide.

camilamacedo86 · 2020-07-08T08:03:17Z

website/content/en/docs/olm-integration/user-guide.md

+ The default version which is installed in the cluster, can be specified in the `Makefile`. The OLM manifests for the default version specified in the Makefile are pre-fetched and present inside the SDK repository as Go source code. Modifying the `OLM_VERSION` variable in Makefile and running `make olm-manifests` will update the current source code file `internal/olm/olm-bindata-x.x.x`, where `x.x.x` refers to the specified version of OLM manifests.
+


The default version which is installed in the cluster, can be specified in the Makefile

How users can specify in their project Makefile the default OLM version? It is specified in the SDK's project Makefile and shipped with its binary. Users have not the SDK Makefile.

Note that end-users have not the make olm-manifests. It can be part of the contributions guide but not of the user-guide.

Yeah, you're right. This doesn't seem to be the right place. Would it be suitable to include this in developers guide instead?

The developer guide makes sense to me.

camilamacedo86 · 2020-07-08T08:05:56Z

website/content/en/docs/cli/operator-sdk_olm_install.md

@@ -19,7 +19,7 @@ operator-sdk olm install [flags]
  -h, --help                   help for install
      --olm-namespace string   namespace where OLM is to be installed. (default "olm")
      --timeout duration       time to wait for the command to complete before failing (default 2m0s)
-      --version string         version of OLM resources to install (default "latest")
+      --version string         version of OLM resources to install; non-defaultversions are downloaded from GitHub.


It is not very clear for me.

Suggested change

--version string version of OLM resources to install; non-defaultversions are downloaded from GitHub.

--version string version of OLM resources to install. Note that, if this value is informed then the installation will be made via the manifests which are downloaded from the OLM repository E.g <add example>.

I'd rather keep this short:

Suggested change

--version string version of OLM resources to install; non-defaultversions are downloaded from GitHub.

--version string version of OLM resources to install (from a Github release); if not set, a pre-packaged OLM version will be installed

Also we can set this default using the internally-set OLM version.

camilamacedo86 · 2020-07-08T08:08:39Z

changelog/fragments/add-deafult-olm-version.yaml

@@ -0,0 +1,7 @@
+entries:
+  - description: >
+      Include a default version of OLM manifests with SDK. Add Makefile target to enable users to 


The Makefile target added is SDK and the user's projects scaffolded was not changed at all. So, this description is not accurate.

camilamacedo86 · 2020-07-08T08:12:10Z

hack/lib/update_olm_manifest.sh

@@ -0,0 +1,35 @@
+#!/usr/bin/env bash


the hack/lib/ is used to add libs which are valid for others scripts. E.g common.sh which is imported in other scripts and has methods which are helpers.

Otherwise, this script is used to gen/build the OLM manifests. Am I right?
So, it should not be in the /hack/lib/. IMO it would be in hack/generate and might should not better a name as generate_olm_manifests.sh?

Makefile

camilamacedo86

See the comments.

hack/lib/update_olm_manifest.sh

estroz · 2020-07-08T17:14:59Z

hack/lib/update_olm_manifest.sh

+filename="olm-bindata-"$1.go
+echo $filename
+
+function get_olm_manifests() {


This function should default to getting the latest release of OLM. If a broken OLM release occurs, we can simply not update bindata or pin to a later non-broken release.

@joelanford had suggested that instead of defaulting to the latest release always, it would be better if we could control the version of OLM which is going to get packaged with SDK. It would be a part of release process (an additional step) to run make olm-manifests to update the latest release we want to include after checking that it isn't broken.

openshift-ci-robot · 2020-07-11T09:51:28Z

@varshaprasad96: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

varshaprasad96 · 2020-09-30T02:49:42Z

Closing this PR in favor of #3906

openshift-ci-robot requested review from estroz and shawn-hurley June 26, 2020 02:22

varshaprasad96 force-pushed the package-olm-manifest branch from ddaaa41 to 55c188b Compare June 26, 2020 02:45

varshaprasad96 requested a review from jmccormick2001 July 1, 2020 17:48

joelanford requested changes Jul 1, 2020

View reviewed changes

cmd/operator-sdk/olm/install.go Outdated Show resolved Hide resolved

internal/olm/client.go Outdated Show resolved Hide resolved

internal/olm/client.go Outdated Show resolved Hide resolved

internal/olm/manager.go Outdated Show resolved Hide resolved

jmccormick2001 approved these changes Jul 1, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2020

camilamacedo86 reviewed Jul 4, 2020

View reviewed changes

varshaprasad96 force-pushed the package-olm-manifest branch from 55c188b to df0028c Compare July 8, 2020 05:17

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2020

[olm-cmd] Add makefile target and relevant documentation

ae4a100

varshaprasad96 force-pushed the package-olm-manifest branch from df0028c to ae4a100 Compare July 8, 2020 05:23

camilamacedo86 reviewed Jul 8, 2020

View reviewed changes

Makefile Show resolved Hide resolved

camilamacedo86 suggested changes Jul 8, 2020

View reviewed changes

camilamacedo86 reviewed Jul 8, 2020

View reviewed changes

hack/lib/update_olm_manifest.sh Show resolved Hide resolved

estroz reviewed Jul 8, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2020

varshaprasad96 closed this Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[olm-cmd] Package SDK with a default version of OLM #3297

[olm-cmd] Package SDK with a default version of OLM #3297

varshaprasad96 commented Jun 26, 2020 •

edited

varshaprasad96 commented Jun 26, 2020

joelanford left a comment

jmccormick2001 left a comment

camilamacedo86 Jul 4, 2020

varshaprasad96 Jul 8, 2020

camilamacedo86 Jul 8, 2020

varshaprasad96 commented Jul 8, 2020

camilamacedo86 Jul 8, 2020 •

edited

camilamacedo86 Jul 8, 2020 •

edited

varshaprasad96 Jul 8, 2020 •

edited

estroz Jul 8, 2020

camilamacedo86 Jul 8, 2020 •

edited

estroz Jul 8, 2020

camilamacedo86 Jul 8, 2020

camilamacedo86 Jul 8, 2020 •

edited

camilamacedo86 left a comment

estroz Jul 8, 2020

varshaprasad96 Jul 8, 2020

openshift-ci-robot commented Jul 11, 2020

varshaprasad96 commented Sep 30, 2020

	--version string version of OLM resources to install; non-defaultversions are downloaded from GitHub.
	--version string version of OLM resources to install. Note that, if this value is informed then the installation will be made via the manifests which are downloaded from the OLM repository E.g <add example>.

[olm-cmd] Package SDK with a default version of OLM #3297

[olm-cmd] Package SDK with a default version of OLM #3297

Conversation

varshaprasad96 commented Jun 26, 2020 • edited

varshaprasad96 commented Jun 26, 2020

joelanford left a comment

Choose a reason for hiding this comment

jmccormick2001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 commented Jul 8, 2020

camilamacedo86 Jul 8, 2020 • edited

Choose a reason for hiding this comment

camilamacedo86 Jul 8, 2020 • edited

Choose a reason for hiding this comment

varshaprasad96 Jul 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Jul 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Jul 8, 2020 • edited

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 11, 2020

varshaprasad96 commented Sep 30, 2020

varshaprasad96 commented Jun 26, 2020 •

edited

camilamacedo86 Jul 8, 2020 •

edited

camilamacedo86 Jul 8, 2020 •

edited

varshaprasad96 Jul 8, 2020 •

edited

camilamacedo86 Jul 8, 2020 •

edited

camilamacedo86 Jul 8, 2020 •

edited