Skip to content

Deploy OpenWhisk API Gateway#10

Closed
kameshsampath wants to merge 6 commits into
projectodd:simplify-deployment-openshiftfrom
kameshsampath:simplify-deployment-openshift
Closed

Deploy OpenWhisk API Gateway#10
kameshsampath wants to merge 6 commits into
projectodd:simplify-deployment-openshiftfrom
kameshsampath:simplify-deployment-openshift

Conversation

@kameshsampath
Copy link
Copy Markdown

OpenShift resources to deploy OpenWhisk API Gateway to Openshift

@jcrossley3 jcrossley3 self-requested a review February 13, 2018 13:57
Copy link
Copy Markdown
Member

@jcrossley3 jcrossley3 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Would you mind renaming the files to *.yml please?

@kameshsampath
Copy link
Copy Markdown
Author

@jcrossley3 - renamed the files to .yml

Copy link
Copy Markdown
Member

@bbrowning bbrowning left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've made some review requests - let me know if you need help figuring this out. Specifically, removing the need to hardcode hostnames and URL may take some thought to figure out.

Comment thread openshift/apigateway.yml Outdated
kind: ConfigMap
metadata:
name: apigateway
namespace: openwhisk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Namespace should be left off here like the other OpenShift configs.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done!

Comment thread openshift/apigateway.yml Outdated
name: apigateway
namespace: openwhisk
data:
api_host: openwhisk-openwhisk.192.168.64.58.nip.io
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

api_host, apigw_url, and redis_host don't need to be hardcoded here. They need to be figured out dynamically in the case of api-host and apigw_url and perhaps set to just redis for redis_host.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@bbrowning - actually I thought to deduce that, but don't know how we can do that - if we can do that then I don't need to have those api_host and apigw_url entries. Thoughts ?

Comment thread openshift/apigateway.yml Outdated
labels:
app: redis
name: redis
namespace: openwhisk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave namespace off here as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread openshift/apigateway.yml Outdated
labels:
app: redis
name: redis
namespace: openwhisk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave namespace off.

Comment thread openshift/apigateway.yml Outdated
name: apigateway
labels:
name: apigateway
namespace: openwhisk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave namespace off.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread openshift/apigateway.yml Outdated
kind: DeploymentConfig
metadata:
name: apigateway
namespace: openwhisk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave namespace off.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread openshift/apigateway.yml Outdated
labels:
app: apigateway
name: apigateway
namespace: openwhisk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave namespace off.

@@ -0,0 +1,39 @@
apiVersion: batch/v1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo in this filename - should be apimanagement.yml

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed. thanks

Comment thread openshift/apimangement.yml Outdated
kind: Job
metadata:
name: install-apimanagement
namespace: openwhisk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave namespace off.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Kamesh Sampath <kamesh.sampath@hotmail.com>
Signed-off-by: Kamesh Sampath <kamesh.sampath@hotmail.com>
- Made the configmap to deduce or infer the values from env

Signed-off-by: Kamesh Sampath <kamesh.sampath@hotmail.com>
…/incubator-openwhisk-deploy-kube into simplify-deployment-openshift
@kameshsampath
Copy link
Copy Markdown
Author

@bbrowning - i have fixed all the comments that you had. Please review and merge when you are at it . Currently there will be one parameter REDIS_HOST whose values I need to have fully qualified because of minishift bug minishift/minishift#2070, once that bug is fixed we can remove the FQDN of REDIS_HOST

@kameshsampath
Copy link
Copy Markdown
Author

Opened new PR projectodd/openwhisk-openshift#4 and will track it there.

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.

3 participants