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.

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

Separate routemgmt.yml out from openwhisk.yml playbook #3760

Merged
merged 4 commits into from Aug 9, 2018

Conversation

rabbah
Copy link
Member

@rabbah rabbah commented Jun 13, 2018

API gateway is missing from redo aka wskdev. Add it and also separate the wsk download into its own playbook. Fix bug in script which failed silently if binary did not exist.

Description

Related issue and scope

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@rabbah rabbah force-pushed the apigw branch 3 times, most recently from d957511 to 58c145d Compare June 13, 2018 18:30
@codecov-io
Copy link

codecov-io commented Jun 13, 2018

Codecov Report

Merging #3760 into master will decrease coverage by 4.7%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3760      +/-   ##
=========================================
- Coverage      76%   71.3%   -4.71%     
=========================================
  Files         145     145              
  Lines        7039    7039              
  Branches      424     424              
=========================================
- Hits         5350    5019     -331     
- Misses       1689    2020     +331
Impacted Files Coverage Δ
...core/database/cosmosdb/RxObservableImplicits.scala 0% <0%> (-100%) ⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala 0% <0%> (-95.1%) ⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala 0% <0%> (-92.6%) ⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala 0% <0%> (-81.82%) ⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala 0% <0%> (-58.83%) ⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala 92% <0%> (-4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35cf728...7acfb71. Read the comment docs.

@rabbah rabbah force-pushed the apigw branch 2 times, most recently from de4745b to e20dce5 Compare June 13, 2018 19:43
@rabbah rabbah added the wip label Jun 13, 2018
@rabbah rabbah added review Review for this PR has been requested and yet needs to be done. and removed wip labels Jun 14, 2018
@rabbah rabbah requested a review from mdeuser June 14, 2018 00:54
@rabbah
Copy link
Member Author

rabbah commented Jun 14, 2018

@mdeuser can you take a look at this PR - I separated the routemgmt package installation as discussed on Slack.

su vagrant -c 'ansible-playbook -i environments/vagrant openwhisk.yml -e invoker_use_runc=False'
su vagrant -c 'ansible-playbook -i environments/vagrant apigateway.yml'
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe even move after postdeploy.yml?

The playbooks `wipe.yml` and `postdeploy.yml` should be run on a fresh deployment only, otherwise all transient
data that include actions and activations are lost.
The playbooks `wipe.yml` should be run on a fresh deployment only, otherwise all data are lost, including actions and activations.
The `postdeploy.yml` is generally run once after a fresh deployment to install a catalog of generally useful packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Run postdeploy.yml after deployment to install a catalog of useful packages"

@mdeuser
Copy link
Contributor

mdeuser commented Jun 15, 2018

@rabbah - i think you have it all nicely covered. the only concern is the breaking change for folk who expect the api web actions to be installed without the extra step. just a communication thing though..

@rabbah
Copy link
Member Author

rabbah commented Jun 17, 2018

Right they'll break if they're using the gateway - The cli repo, and api gw repos may thus require tweaks once this PR is merged.

An alternative is to have the apigateway playbook install the routemgmt package - which makes sense to me since you can't use the gateway without the package. For deployment that want to use a different gateway, they then can execute the independent routemgmt playbook separately.

@rabbah
Copy link
Member Author

rabbah commented Jun 17, 2018

@mdeuser I pushed a commit to merge the routemgmt playbook installation with managing the API gateway itself. This should backward compatible. If someone wasn't using the apigw playbook then they will need to add the routemgmt to their deployment.

@csantanapr
Copy link
Member

@mdeuser can you take this reviewed it, this might have impacts to our downstream CI for the ordering of gateway.yml playbook now needs to be done after openwhisk.yml
Also apigateway repo would need to be adjusted here https://github.com/apache/incubator-openwhisk-apigateway/blob/master/tools/travis/build.sh#L51

rabbah added a commit to rabbah/incubator-openwhisk-apigateway that referenced this pull request Jun 21, 2018
In preparation for a reorg in this PR apache/openwhisk#3760.
@rabbah
Copy link
Member Author

rabbah commented Jun 21, 2018

@csantanapr @mdeuser I created this PR apache/openwhisk-apigateway#314 which should be OK to merge now (Travis will verify) and will work after this PR is merged.

@rabbah
Copy link
Member Author

rabbah commented Jul 3, 2018

@mdeuser anything for me to address on this PR?

@rabbah
Copy link
Member Author

rabbah commented Jul 5, 2018

@mdeuser is there a repo I need to fix downstream you’re concerned about?

@mdeuser
Copy link
Contributor

mdeuser commented Jul 6, 2018

@rabbah - my apologies for the delay. i've been in interrupt mode recently.. would you be ok with leaving the routemgmt.yml outside of the apigateway.yml.

@rabbah
Copy link
Member Author

rabbah commented Jul 6, 2018

does it make sense to do that - can you explain the rationale? My thinking is:

  • if someone deploys the gateway, they need the route management package.
  • if someone doesn't deploy the gateway, they have the option of not running the playbook (which avoids installing the route management package as well).
  • if someone is using a third party gateway, they just run the playbook to install the route management package.

@mdeuser
Copy link
Contributor

mdeuser commented Jul 6, 2018

i guess i was seeing if we can avoid, as much as possible, changes that require some folks to tweak their deployment procedures. currently, the apigateway.yml deployment has been agnostic as to whether or not openwhisk has been deployed; by including routemgmt in that .yml, it now requires openwhisk to have been deployed. also, any ci/cd pipelines that use a local api gw deployment in some environments, but use an already deployed production api gw in other environments will need new conditional logic when deploying openwhisk to avoid deploying routemgmt twice - a second deployment won't cause a failure, just would take some extra time.

@rabbah
Copy link
Member Author

rabbah commented Jul 6, 2018

By pulling out routemgmt.yml from the openwhisk playbook there's already a need to adjust a deployment: if you need the package, you have to add it to the deployment explicitly. So it's move the apigw playbook down, or leave it where it is and then add a second playbook. I'm not really seeing a big benefit here since either way - there's a change.

Also none of our playbooks can be deployed again without tear down - what this PR does is fix the bug in the playbook so you can properly uninstall and reinstall the packages.

@rabbah
Copy link
Member Author

rabbah commented Jul 6, 2018

As in this PR, this is not a big change:
https://github.com/apache/incubator-openwhisk-apigateway/pull/314/files#diff-183ae0496c3df4528d793134f32a6955R50

Maybe there are other scenarios I'm missing.

@mdeuser
Copy link
Contributor

mdeuser commented Jul 6, 2018

By pulling out routemgmt.yml from the openwhisk playbook there's already a need to adjust a deployment

true. but imho only needing to make the routemgmt.yml change is a bit simpler in a dynamic pipeline than handling the combined apigw/routemgmt. the routemgmt is always needed - no need for conditional deployment checks based on the target environment.

@rabbah
Copy link
Member Author

rabbah commented Jul 6, 2018

I don't follow - are you saying you have a pipeline where you deploy the apigateway once but the routemgmt package many times?

@rabbah
Copy link
Member Author

rabbah commented Jul 6, 2018

Noting result of discussion on slack: reverted the last commit. The rationale is that the apigw playbook is deploying a component (like couchdb) and routemgmt is related to configuration (like initdb/wipedb) and consistent with how we handle the lifecycle for those component, the two playbooks herein should be separate.

Thanks @mdeuser for the debate.

@mdeuser
Copy link
Contributor

mdeuser commented Jul 17, 2018

pg4/1965

Copy link
Contributor

@mdeuser mdeuser left a comment

Choose a reason for hiding this comment

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

changes look good to me

@rabbah
Copy link
Member Author

rabbah commented Jul 24, 2018

@mdeuser status?

Update documentation for deployment.
Add API gateway to redo.
The routemgmt playbook remains its own independent playbook so that it may be installed seperately when necessary.
@csantanapr
Copy link
Member

@mdeuser Can this be merge?

@csantanapr csantanapr changed the title Add API gateway to redo. Separate routemgmt.yml out from openwhisk.yml playbook Aug 9, 2018
@mdeuser mdeuser merged commit 9ae7a84 into apache:master Aug 9, 2018
@rabbah rabbah deleted the apigw branch February 8, 2019 02:29
BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
* Seperate route management package for API gateway from openwhisk.yml.

Update documentation for deployment.
Add API gateway to redo.

* Install routemgmt package when running API gateway playbook.

The routemgmt playbook remains its own independent playbook so that it may be installed seperately when necessary.

* Revert "Install routemgmt package when running API gateway playbook."

This reverts commit ae278f6.

* Fix Ruby example to use puts, adding a new line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment review Review for this PR has been requested and yet needs to be done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants