add remote_redirect mode for openwhisk_cli installation #3100
Conversation
Add a third remote_redirect option to the prior local and remote options for cli installation. In remote_redirect mode, the CLI binaries are not installed into the edge server's file system. All requests to download the CLI are instead redirected to an external URL (for example to download from the openwhisk-cli release page). This follows the pattern already set for the iOS starter app and blackbox SDK. See discussion in issue apache#2152.
|
Closes #2152 This is a rework of PR #3093 based on feedback from @csantanapr |
|
PG1 2454 - timed out during testing overnight. Trying again. |
|
needs labels: review, companion. |
rabbah
added
review
|
@csantanapr @houshengbo -- reminder to review |
There was a problem hiding this comment.
No doubt that this will work, if it's merged before #3133, and I can make the appropriate changes to it; however, I think we should separate the Nginx-related configuration (making the CLI available for download) from the sourcing of bin/wsk for 'local' execution within the cluster.
| {% if openwhisk_cli.installation_mode == "remote_redirect" %} | ||
| # redirect requests for specific binaries to the matching one from the {{ openwhisk_cli_tag }} openwhisk-cli release. | ||
| {% for os in cli_os %} | ||
| {% for arch in cli_arch %} |
There was a problem hiding this comment.
This concerns me given changes I made in #3133 to introduce cli_os_arch. It will be a simple update, depending on which goes first.
| @@ -26,3 +27,7 @@ | |||
| when: cli_installation_mode == "local" | |||
|
|
|||
| - include: download_cli.yml | |||
| when: cli_installation_mode != "remote_redirect" | |||
There was a problem hiding this comment.
My gut says this should be cli_installation_mode == "remote" because selecting local install is usually because the download is broken for a particular platform. Of course, I'm not sure local is functionally complete in the configuration. But, more to the point, we should separate making the CLI available for download via NGinx from sourcing the binary for /wsk/bin for local use.
|
|
||
| - name: "set the file name to download for linux" | ||
| set_fact: | ||
| cli_download_name={{ openwhisk_cli.remote_redirect.package_name }}-{{ openwhisk_cli_tag }}-linux-amd64.tgz |
There was a problem hiding this comment.
This is a bit more complicated now, because there will be multiple linux architectures (amd64, s390x). Is there a reason we can't reuse the existing ansible files for downloading the cli? It seems configuring Nginx should be a separate problem from downloading a binary for "local" use.
|
Thinking about this change more over break, it seems to me that it would be cleaner to not conflate local building vs. downloading of the CLU and embedding the CLI in the edge host vs. putting a redirect in the nginx.conf file. So instead of having 3 modes for cli_installation_mode we would have two boolean properties: cli_build_locally (default to false) and cli_download_redirect (default to true). Thoughts? |
|
I was heading in the same direction. My intention was to have build occur in the 'incubator-openwhisk-cli' project. The build would actually create two artifacts:
Riffing on that, the CLI "download" could pull the tarball from a URL, and for local build the configuration could override that URL with |
|
What do we want to do with this PR? Should I close it in favor of the work @jonpspri is doing, or is there some piece of it that is worth pulling as a separate PR that complements his cli changes? |
|
@csantanapr ^^ WDYT wrt @dgrove-oss 's comment? How do we want to proceed? |
|
I like @jonpspri proposal
Closing this PR, if @jonpspri needs something from this PR he is free to take it here. |
Add a third remote_redirect option to the prior local and
remote options for cli installation. In remote_redirect mode,
the CLI binaries are not installed into the edge server's
file system. All requests to download the CLI are instead
redirected to an external URL (for example to download
from the openwhisk-cli release page). This follows the
pattern already set for the iOS starter app and blackbox SDK.
See discussion in issue #2152.