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

Update deprovision test case in test.js #41

Closed
wants to merge 1 commit into from
Closed

Update deprovision test case in test.js #41

wants to merge 1 commit into from

Conversation

leonwanghui
Copy link
Collaborator

Signed-off-by: leonwanghui wanghui71leon@gmail.com

This patch is proposed mainly for resolving issue #33 with some changes below:

  • Add some configuration of deprovision operation in config_mock.json
  • Remove some blank spaces in config_mock.json
  • Change binding to provision in test.js

Signed-off-by: leonwanghui <wanghui71leon@gmail.com>
@cfdreddbot
Copy link

Hey leonwanghui!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@leonwanghui leonwanghui mentioned this pull request Jul 30, 2018
@zhongyi-zhang
Copy link
Collaborator

zhongyi-zhang commented Sep 30, 2018

This PR looks good to solve the issue.
But in a higher view, I do think the config structure needs a redesign to make more sense, so that broker authors can customize it easier. The running order for these test cases is not obvious, e.g. deprovision is mixed into provisions in current config file but actually it runs after the binding cases below.
My proposal is that: each case focus on a service with a plan. Broker authors customize only a valid lifecycle and optional validations. For error-expected cases, OSB checker should be able to generate them e.g. the "conflict" scenario case in current config file -- it's not hard to generate invalid request body from a author-provided valid body.

Just draft a pseudo case for that:

{
  "name": "my abc service test",
  "service_id" :"blabla",
  "plan_id": "blabla",
  "organization_guid": "org-guid-here",
  "space_guid": "space-guid-here",
  "lifecycle": [
    {
      "operation": "provision",
      "async": true
      "parameters": {
        "blabla": "blabla"
      },
    },
    {
      "operation": "update",
      "async": true
      "parameters": {
        "blabla": "blabla"
      },
    },
    {
      "operation": "bind",
      "parameters": {
        "blabla": "blabla"
      },
      "expectedCredentialSchema": { // optional
        "blablakey": "<string>"
      }
    },
    {
      "operation": "unbind",
    },
    {
      "operation": "deprovision",
      "async": true
    }
  ]
}

Of course test.js would need a refactor to adapt these changes.

@norshtein
Copy link
Collaborator

I agree with @zhongyi-zhang , the new version is more neat and readable.

@zhongyi-zhang
Copy link
Collaborator

Went through other PRs and found that my proposal impacts a lot. I think we can discuss more about it in a new issue. If it is popular, the change will close several PRs and issues.

@leonwanghui
Copy link
Collaborator Author

Agreed, so I will keep this PR opened till the redesign proposal been finished, is that ok?

@leonwanghui leonwanghui closed this Nov 6, 2018
@leonwanghui leonwanghui deleted the test-fixer branch November 6, 2018 03:27
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.

None yet

4 participants