Adding Text Analytics module (Cognitive Services) #557

neilpeterson · 2018-08-24T00:22:14Z

@krancour coming back around to this. This PR adds the Cognitive Services Text Analytics API to OSBA. I had initially submitted a related PR many months back, however that was delayed due to an ongoing refactor of the broker.

Here is the initial PR - #463

I've refactored to align with the current state of the broker.

One recommendation in the initial PR was to shrink this down into a single plan and specify the tier with a provisioning parameter. I am cool with this, but have not done so yet. One of plan properties is Free. Text Analytics has a free tier, and then several others. If using a single plan however would you suggest I use the Free property.

Thanks a bunch for checking this out.

Tests

Here are the results of make test-service-lifecycles:

2018/08/23 23:44:52 ----> creating resource group "test-6652437c-e49b-47f3-943d-b6eaafe0e640"
2018/08/23 23:44:55 ----> created resource group "test-6652437c-e49b-47f3-943d-b6eaafe0e640"
=== RUN   TestServices
2018/08/23 23:44:55 ----> testing in resource group "test-6652437c-e49b-47f3-943d-b6eaafe0e640"
2018/08/23 23:44:55 Running tests for modules: [textanalytics]
=== RUN   TestServices/lifecycle
=== RUN   TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-free
=== PAUSE TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-free
=== RUN   TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s0
=== PAUSE TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s0
=== RUN   TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s1
=== PAUSE TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s1
=== RUN   TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s2
=== PAUSE TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s2
=== RUN   TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s3
=== PAUSE TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s3
=== RUN   TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s4
=== PAUSE TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s4
=== CONT  TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-free
2018/08/23 23:44:55 ----> TestServices/lifecycle/textanalytics/text-analytics-free: starting
=== CONT  TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s4
2018/08/23 23:44:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s4: starting
=== CONT  TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s3
2018/08/23 23:44:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s3: starting
=== CONT  TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s2
2018/08/23 23:44:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s2: starting
=== CONT  TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s1
2018/08/23 23:44:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s1: starting
=== CONT  TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s0
2018/08/23 23:44:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s0: starting
2018/08/23 23:45:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s3: in progress
2018/08/23 23:45:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s4: in progress
2018/08/23 23:45:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s1: in progress
2018/08/23 23:45:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s2: in progress
2018/08/23 23:45:55 ----> TestServices/lifecycle/textanalytics/text-analytics-free: in progress
2018/08/23 23:45:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s0: in progress
2018/08/23 23:46:31 ----> TestServices/lifecycle/textanalytics/text-analytics-s1: completed
2018/08/23 23:46:32 ----> TestServices/lifecycle/textanalytics/text-analytics-free: completed
2018/08/23 23:46:32 ----> TestServices/lifecycle/textanalytics/text-analytics-s0: completed
2018/08/23 23:46:33 ----> TestServices/lifecycle/textanalytics/text-analytics-s3: completed
2018/08/23 23:46:46 ----> TestServices/lifecycle/textanalytics/text-analytics-s4: completed
2018/08/23 23:46:55 ----> TestServices/lifecycle/textanalytics/text-analytics-s2: in progress
2018/08/23 23:47:14 ----> TestServices/lifecycle/textanalytics/text-analytics-s2: completed
--- PASS: TestServices (139.27s)
    --- PASS: TestServices/lifecycle (0.00s)
        --- PASS: TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s1 (96.44s)
        --- PASS: TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-free (96.87s)
        --- PASS: TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s0 (97.35s)
        --- PASS: TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s3 (98.58s)
        --- PASS: TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s4 (111.62s)
        --- PASS: TestServices/lifecycle/TestServices/lifecycle/textanalytics/text-analytics-s2 (139.26s)
PASS
2018/08/23 23:47:14 ----> deleting resource group "test-6652437c-e49b-47f3-943d-b6eaafe0e640"
2018/08/23 23:47:16 ----> deleted resource group "test-6652437c-e49b-47f3-943d-b6eaafe0e640"
ok      github.com/Azure/open-service-broker-azure/tests/lifecycle      143.266s

jeremyrickard · 2018-09-18T15:52:53Z

@neilpeterson I've gone through this and I like the addition, thanks!

While running CI, it seems there is an issue w/ the vendored code. Could you ensure that the dep config/vendor is updated? make verify-vendored-code will help verify that

neilpeterson · 2018-09-18T17:46:54Z

@jeremyrickard - thanks. I've run make dep which seems to have cleared up the issue.

I'm not super familiar with package management in Go, if I've done something incorrect here, I apologize :). If so, can you provide a pointer?

Thanks

tests/lifecycle/textanalytics_cases_test.go

norshtein · 2018-09-19T03:06:18Z

pkg/services/textanalytics/plan_schemas.go

+				Description: "The (new or existing) resource group with which" +
+					" to associate new resources.",
+			},
+			"name": &service.StringPropertySchema{


Why make parameter name user-specified? In other modules, the name of the server, database, and so on, is generated randomly and is not configurable for users. I'm not familiar with textanalytics, please tell me if there is any special concern to do this. If not, could you make name randomly generated just like other modules?

Thanks for catching this @norshtein . The parameter was actually unused and has been removed.

neilpeterson · 2018-09-21T21:43:51Z

@jeremyrickard @norshtein @zhongyi-zhang I've address all noted issues. If you need me to do anything else, please let me know.

Thanks

tests/lifecycle/textanalytics_cases_test.go

zhongyi-zhang

LGTM

norshtein

LGTM

norshtein · 2018-09-25T05:30:16Z

Since new merged PR has modified the Gopkg.lock, I'm afraid you have to run make dep again to solve the vendored code issue, sorry for inconvenience. Also, please have a look on lint issue, thanks!

neilpeterson · 2018-09-26T15:50:00Z

Thanks @norshtein - I've run make dep and have also fixed up the lint issues.

zhongyi-zhang · 2018-09-28T08:02:20Z

@neilpeterson thanks for the contribution! This service is a good first experimental service of that a bunch of Cognitive Services in OSBA.

neilpeterson · 2018-09-28T08:11:27Z

@zhongyi-zhang - thanks a bunch for your help on this one.

Adding Text Analytics module (cognitve services)

54a7e2c

neilpeterson requested review from jeremyrickard and krancour as code owners August 24, 2018 00:22

neilpeterson added 2 commits August 23, 2018 17:34

Merge branch 'master' into rename-module-to-text-analytics

2e1c0d1

Merge branch 'master' into rename-module-to-text-analytics

5abbd76

krancour added new module low priority labels Sep 13, 2018

neilpeterson and others added 3 commits September 13, 2018 12:28

Merge branch 'master' into rename-module-to-text-analytics

a47d844

Merge branch 'master' into rename-module-to-text-analytics

321a8d6

Merge branch 'master' into rename-module-to-text-analytics

7606d13

neilpeterson added 2 commits September 18, 2018 10:39

ran make dep

bf08e76

Fix merge conflict

d5a2b07

zhongyi-zhang reviewed Sep 19, 2018

View reviewed changes

tests/lifecycle/textanalytics_cases_test.go Show resolved Hide resolved

norshtein reviewed Sep 19, 2018

View reviewed changes

Removed unused parameter and added credentials test

0ee48d1

Merge branch 'master' into rename-module-to-text-analytics

1e17a0d

zhongyi-zhang reviewed Sep 24, 2018

View reviewed changes

tests/lifecycle/textanalytics_cases_test.go Show resolved Hide resolved

Merge branch 'master' into rename-module-to-text-analytics

b8ff1c9

zhongyi-zhang previously approved these changes Sep 24, 2018

View reviewed changes

Merge branch 'master' into rename-module-to-text-analytics

aff4b5a

norshtein previously approved these changes Sep 25, 2018

View reviewed changes

Ran make dep and fixed lint issues

ae36d00

neilpeterson dismissed stale reviews from norshtein and zhongyi-zhang via ae36d00 September 26, 2018 15:48

norshtein approved these changes Sep 27, 2018

View reviewed changes

zhongyi-zhang merged commit 9597a22 into Azure:master Sep 28, 2018

neilpeterson deleted the rename-module-to-text-analytics branch September 28, 2018 08:11

zhongyi-zhang mentioned this pull request Sep 28, 2018

Add a link for Text Analytics service in the home README #590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Text Analytics module (Cognitive Services) #557

Adding Text Analytics module (Cognitive Services) #557

neilpeterson commented Aug 24, 2018

jeremyrickard commented Sep 18, 2018 •

edited

Loading

neilpeterson commented Sep 18, 2018

norshtein Sep 19, 2018

neilpeterson Sep 21, 2018

neilpeterson commented Sep 21, 2018

zhongyi-zhang left a comment

norshtein left a comment

norshtein commented Sep 25, 2018

neilpeterson commented Sep 26, 2018

zhongyi-zhang commented Sep 28, 2018

neilpeterson commented Sep 28, 2018

Adding Text Analytics module (Cognitive Services) #557

Adding Text Analytics module (Cognitive Services) #557

Conversation

neilpeterson commented Aug 24, 2018

Tests

jeremyrickard commented Sep 18, 2018 • edited Loading

neilpeterson commented Sep 18, 2018

norshtein Sep 19, 2018

Choose a reason for hiding this comment

neilpeterson Sep 21, 2018

Choose a reason for hiding this comment

neilpeterson commented Sep 21, 2018

zhongyi-zhang left a comment

Choose a reason for hiding this comment

norshtein left a comment

Choose a reason for hiding this comment

norshtein commented Sep 25, 2018

neilpeterson commented Sep 26, 2018

zhongyi-zhang commented Sep 28, 2018

neilpeterson commented Sep 28, 2018

jeremyrickard commented Sep 18, 2018 •

edited

Loading