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

Swift 4.0.2 support, CloudEnv migration and Kubernetes Support #40

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

KyeMaloy97
Copy link
Contributor

  • Updated Dockerfile and Travis to Swift 4.0.2, removed Character references in code
  • Removed SwiftJSON support as Kitura 2 no longer uses it, standard JSONEncoder is now used instead.
  • Added support for Armada instead of Kracken @ShihabMehboob
  • Updated the config.sh to add support for Kubernetes @ShihabMehboob
  • Added some credential JSON files

This PR is instead of the two smaller ones, #37 and #38 , as these two smaller PRs had some code that was shared, especially in the Server side of the application. Passing on Linux on Travis as of this, waiting for Mac to complete building and testing.

@KyeMaloy97 KyeMaloy97 requested a review from sandmman November 22, 2017 18:23
sandmman
sandmman previously approved these changes Nov 22, 2017
Copy link

@sandmman sandmman 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!

config.sh Outdated
cf api https://api.ng.bluemix.net
cf login
cf ic login
bx login --sso -a api.eu-gb.bluemix.net
Copy link
Member

Choose a reason for hiding this comment

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

--sso applies only to internal users who are using federated ID - fine for our testing purposes, but must be removed.

Is there a reason why eu-gb is now default?

Copy link
Member

Choose a reason for hiding this comment

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

I would like the api defintion here, and the registry definitions to be dynamic - change it at the top of the file.

config.sh Outdated
populate-db <imageName> Populates database with initial data
delete <imageName> Delete the group container and deletes created service if possible
delete <imageName> Delete the created service and cluster if possible
all <imageName> Combines all necessary commands to deploy an app to IBM Cloud in a Docker container.
Copy link
Member

Choose a reason for hiding this comment

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

The help commands need to be updated - all prompts me for multiple args, and I do not know what the second one is.

@christiancompton christiancompton dismissed sandmman’s stale review November 27, 2017 13:11

There are a couple of changes needed in the Kubernetes scripts.

config.sh Outdated
@@ -36,31 +38,53 @@ install-tools () {
brew tap cloudfoundry/tap
brew install cf-cli
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this irrelevant given the bx cli migration?

Copy link
Member

Choose a reason for hiding this comment

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

We should install bx cli here.

config.sh Outdated
return
fi
docker build -t $1 --force-rm .
docker build -t registry.eu-gb.bluemix.net/$NAME_SPACE/todolist-couchdb .
Copy link
Member

Choose a reason for hiding this comment

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

This should be dynamic using the registry definition at the top of the file

config.sh Outdated
docker tag $1 $REGISTRY_URL/$namespace/$1
docker push $REGISTRY_URL/$namespace/$1
bx cr login
docker push registry.eu-gb.bluemix.net/$NAME_SPACE/todolist-couchdb
Copy link
Member

Choose a reason for hiding this comment

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

This should be dynamic using the registry definition at the top of the file

config.sh Outdated
cd ..
rm -rf $BRIDGE_APP_NAME
deployContainer () {
kubectl run todo-deployment --image=registry.eu-gb.bluemix.net/$NAME_SPACE/todolist-couchdb
Copy link
Member

Choose a reason for hiding this comment

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

This should be dynamic using the registry definition at the top of the file

config.sh Outdated
@@ -36,31 +38,53 @@ install-tools () {
brew tap cloudfoundry/tap
brew install cf-cli
cf install-plugin https://static-ice.ng.bluemix.net/ibm-containers-mac
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

config.sh Outdated
@@ -53,7 +53,7 @@ config-cli () {

login () {
echo "Setting api and login tools."
bx login --sso -a api.eu-gb.bluemix.net
bx login -a $LOGIN_URL
Copy link
Member

Choose a reason for hiding this comment

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

One problem with the migration to resource groups is that the Org/Space targeting is no longer interactive.

Copy link
Member

Choose a reason for hiding this comment

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

This can be resolved by adding bx target --cf below this line.

Copy link
Member

@christiancompton christiancompton left a comment

Choose a reason for hiding this comment

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

It is unclear - are imagename and "instance name" the same? They are being used interchangeably.

@ShihabMehboob
Copy link

ShihabMehboob commented Dec 4, 2017

The script now works end-to-end (tested multiple times), yet there may still be a small issue in the setup function where creating a new cluster takes time to deploy, a wait time of 5 minutes has been placed to account for this but the deployment may not finish in that time and the script could fail as a result. With internal testing, it will also fail if a cluster already exists as only one is allowed per user. The set -e command has been added to end the execution of the script if it fails, however this may need to be removed as the setup function often fails due to the cluster deployment taking too long, or discovering that a cluster already exists.

@christiancompton
Copy link
Member

christiancompton commented Dec 5, 2017

Nice Work @ShihabMehboob! A few more points to address:

  • The sleep/cluster creation is too unreliable - extract the cluster-create & similar lines to be a separate function, not invoked by the all command.
  • In the readme, describe this as a prerequisite for using the all and individual commands, including the command to verify that the worker nodes have deployed successfully and can be used.
  • @sandmman found this problem: The script uses the cluster name as the service/deployment name inside the cluster. But the service/deployment names should be lowercase and without -s, which clusters often have.

The script still does not work for me - the export is not functioning as expected. I suspect that it is because the export command is hardcoded to use the par01 data center. Can you validate that this works if you change the endpoint to be one other than eu-gb? I think if you change the corresponding values at the top, you will experience the same problems.

One final change:

  • The constraint about US-SOUTH environment is no longer valid - we should use that as our default endpoint since far more users use that geography.

@ShihabMehboob
Copy link

Addressed all the points and tested again, works fine. The region has been changed to US-SOUTH as well now. Amended the export command to get the datacenter from the passed in cluster, and isolated the setup function with a short excerpt in the readme about it being a prerequisite.

@christiancompton
Copy link
Member

christiancompton commented Dec 5, 2017

This script does not work for brand new users in its current form - setup() currently assumes that the bluemix command line, container registry, etc. are installed, but we do that in the install_tools command - which would be run after setup.

Since this presents a problem, let's do the following:

  • add setup back into the all method
  • Removing the arbitrary sleep value
  • requisite readme changes - prereqs and steps
  • In prereq section, link to the containers UI IBM Cloud page, and say that existing clusters can be used by the script, and warn users that it can take a while to be operational
  • add a check to see if a cluster by that name already exists, and if not, create one if bx cs / cluster-get <NAME> / then / echo "Cluster not found, attempting to create new cluster..." / bx cs cluster-create <NAME> / fi
  • after the logic above, add a variation of the following polling mechanism:
until bx cs cluster-get NAME | grep normal
do
    sleep 60
    bx cs cluster-get NAME
    echo "Cluster still provisioning..."
done

echo "Cluster deployed successfully"

I have already validated that these commands will work, even if the bx cli is set to another locale (pt_BR & ko_KR).

@christiancompton
Copy link
Member

christiancompton commented Dec 21, 2017

@ShihabMehboob I now see the deployment is now operational, and the app no longer crashes.

To access the app, the NodePort must be exposed in order for the IP to be retrieved. i.e. kubectl expose deployment --type=NodePort. This may impact the run command currently in the script.

This should make an end to end test possible - all(), populate(), & delete(). Once this flow is verified, the PR can be merged.

@sandmman
Copy link

@ShihabMehboob I heard this was fully working now?! Don't forget to remove the values in config/my-cloudant-credentials.json. I noticed they got inserted in a recent commit.

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2018

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ ShihabMehboob
✅ christiancompton
✅ KyeMaloy97
❌ Aaron Liberatore


Aaron Liberatore seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

5 participants