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

Add hostname to test scripts #985

Merged
merged 3 commits into from Jun 19, 2018
Merged

Add hostname to test scripts #985

merged 3 commits into from Jun 19, 2018

Conversation

alaypatel07
Copy link
Contributor

@alaypatel07 alaypatel07 commented Jun 13, 2018

Describe what this PR does and why we need it:

Add hostname variable to test scripts and change the url

Changes proposed in this pull request

  • Add hostname variable in all test scripts and take it as the last command line argument
  • Change the url to reflect new changes

@alaypatel07 alaypatel07 requested a review from jmrodri June 13, 2018 17:05
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 13, 2018
@eriknelson eriknelson self-requested a review June 13, 2018 17:07
Copy link
Contributor

@eriknelson eriknelson left a comment

Choose a reason for hiding this comment

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

Nice job Alay!

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

These look like reasonable changes to me. The only concern I have is that these scripts won't work for a developer deployed broker (make deploy). Not sure how to make this more resilient.

test/bind.sh Outdated
"https://asb-1338-ansible-service-broker.172.17.0.1.nip.io/ansible-service-broker/v2/service_instances/$INSTANCE_ID/service_bindings/$BINDING_ID?accepts_incomplete=true"


"https://asb-openshift-automation-service-broker.$HOSTNAME.nip.io/openshift-automation-service-broker/v2/service_instances/$INSTANCE_ID/service_bindings/$BINDING_ID?accepts_incomplete=true"
Copy link
Member

Choose a reason for hiding this comment

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

This will only work for oc cluster up --enable=automation-service-broker, it won't work in the development use case (ie. after make deploy).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure building out these developer focused scripts dynamically for all our deployment scenarios is in scope. Although to your point, make deploy is probably a developer function, so maybe it deserves attention.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they will not work for a developer then -1 to this change. These scripts are PURELY for hooking up to a developers deployment of the broker.

@coveralls
Copy link

coveralls commented Jun 14, 2018

Coverage Status

Coverage remained the same at 33.435% when pulling 896f38c on alaypatel07:master into 1c5cfb3 on openshift:master.

@jmrodri jmrodri added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 14, 2018
@jmrodri
Copy link
Contributor

jmrodri commented Jun 14, 2018

I will test these out to see if they suit the developer workflow or now.

@jmrodri
Copy link
Contributor

jmrodri commented Jun 14, 2018

These scripts are primarily used by a developer so the default case should work against a make deploy installation.

Developer setup

  • Remove automation-service-broker from catasb
    • In ansible/roles/openshift_setup/tasks/main.yml of catasb remove ,automation-service-broker from the --enable= line.
  --enable=service-catalog,template-service-broker,router,registry,web-console,persistent-volumes,sample-templates,rhel-imagestreams
  • Deploy the cluster using catasb

    • cd local/linux
    • ./reset_environment.sh
  • In the broker repo, deploy it

    • cd $GOPATH/src/github.com/openshift/ansible-service-broker
    • make deploy

I should then be able to run test/catalog.sh against this deployed instance without any special parameters or flags. So I think think the url in the scripts should default to the one used by make deploy.

routes after make deploy

NAME                HOST/PORT                                               PATH      SERVICES            PORT        TERMINATION   WILDCARD
broker              broker-automation-broker.172.17.0.1.nip.io                        automation-broker   port-1338   reencrypt     None
broker-redirector   broker-redirector-automation-broker.172.17.0.1.nip.io             automation-broker   port-1337                 None

The endpoint is /automation-broker just before /v2. For example, the catalog call would look like this: https://broker-automation-broker.172.17.0.1.nip.io/automation-broker/v2/catalog

Catasb "production" setup

When a broker is launched using pure catasb, the url is completely different. I would be okay if we want the scripts to also work with that setup. In that case I would expect pass in a flag to the script to indicate it is a production or catasb setup, i.e. it is not a development setup.

For example, ./test/catalog.sh --prod or something like that. Then in the script if it sees the flag it uses the other route.

routes after catasb

NAME      HOST/PORT                                                   PATH      SERVICES                              PORT        TERMINATION   WILDCARD
asb       asb-openshift-automation-service-broker.172.17.0.1.nip.io             openshift-automation-service-broker   port-1338   reencrypt     None

The endpoint is /openshift-automation-service-broker/ just before /v2. For example, the catalog call would look like this: https://asb-openshift-automation-service-broker.172.17.0.1.nip.io/openshift-automation-service-broker/v2/catalog

The working with catasb "production" setup is OPTIONAL in my opinion.

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

See lengthy comment
#985 (comment)

@jmrodri jmrodri removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 19, 2018
@jmrodri
Copy link
Contributor

jmrodri commented Jun 19, 2018

Random testing looked good. Visual inspection looks good too.

Last operation passed

$ ./last_operation.sh 687932b6-7413-11e8-9c21-0242ac11000a c7e5f58b-d689-4c45-ba39-0d49de81a487 7f4a5e35e4af2beb70076e72fab0b7ff 1dda1477cace09730bd8ed7a6505607e
{
  "state": "succeeded",
  "description": "provision job completed"
}

Get instance passed

$ ./getinstance.sh 687932b6-7413-11e8-9c21-0242ac11000a
{
  "service_id": "687932b6-7413-11e8-9c21-0242ac11000a",
  "plan_id": "dev",
  "parameters": {
    "_apb_last_requesting_user": "admin",
    "_apb_plan_id": "dev",
    "_apb_service_class_id": "1dda1477cace09730bd8ed7a6505607e",
    "_apb_service_instance_id": "687932b6-7413-11e8-9c21-0242ac11000a",
    "postgresql_database": "admin",
    "postgresql_password": "password",
    "postgresql_user": "admin",
    "postgresql_version": "9.6"
  }
}

catalog passed

$ ./catalog.sh
{
  "services": [
    {
      "name": "dh-pyzip-demo-apb",
      "id": "0e991006d21029e47abe71acc255e807",
      "description": "Python Zip Demo APB Implementation",
      "bindable": false,
      "metadata": {
        "dependencies": [
          "docker.io/ansibleplaybookbundle/py-zip-demo:latest"
        ],
        "displayName": "Pyzip Demo (APB)",
        "documentationUrl": "https://github.com/fusor/apb-examples/tree/master/pyzip-demo-apb",
        "longDescription": "APB Implementation of the Python webapp, https://github.com/fusor/py-zip-demo",
        "providerDisplayName": "Red Hat, Inc."
      },
      "instances_retrievable": true,
      "bindings_retrievable": false,
...

bootstrap passed

$ ./bootstrap.sh 
{
  "spec_count": 42,
  "image_count": 112
}

@jmrodri
Copy link
Contributor

jmrodri commented Jun 19, 2018

@djzager I've verified that Alay made the changes you pointed out, they now work with make deploy which is the primary goal for this. I'm going to merge this now.

@jmrodri jmrodri merged commit bb89b21 into openshift:master Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants