[infra-proxy-service] Environments list using search API with pagination #4659

kvivek1115 · 2021-01-28T04:50:42Z

🔩 Description: What code changed, and why?

Environment list API now also work with paganization and searching with the name.
If we won't pass any params it will fetch all environments available in the system.
Search query works with the following request params
- search_query.q: Search query string have to in the format like *:*, name:* or name:chef-env-* etc.
- search_query.page: offset of the records. [Updated 08-Feb-2020]
- search_query.per_page: per page records need to fetch. [Updated 08-Feb-2020]
gRPC request

chef-automate dev grpcurl infra-proxy-service chef.automate.domain.infra_proxy.service.InfraProxyService.GetEnvironments -- -d '{"server_id": "chef-server-dev-test", "org_id": "chef-org-dev", "search_query": {"q": "name:*", "page": 0, "per_page": 1}}'
{
  "environments": [
    {
      "name": "developmen1t"
    }
  ],
  "page": 0,
  "total": 13
}

⛓️ Related Resources

Fixes: #4627

👍 Definition of Done

Environment list API is now available with search and pagination.

👟 How to Build and Test the Change

Steps to build:

rebuild components/infra-proxy-service
rebuild components/automate-gateway

Steps to test:

To add data using 3rd internal steps https://github.com/chef/automate/blob/master/dev-docs/adding-data/adding_test_data.md#adding-data-to-infra-views
Environment list API without filter query

curl -sSkH "api-token: $(get_admin_token)" https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/environments?pretty               
{
  "environments": [
    {
      "name": "developmen1t",
      "description": ""
    },
    {
      "name": "dev3",
      "description": ""
    },
    {
      "name": "_default",
      "description": "The default Chef environment"
    },
    {
      "name": "production",
      "description": ""
    },
   ...
  ],
  "page": 0,
  "total": 13,
}

Environment list API with a filter search query.

curl -sSkH "api-token: $(get_admin_token)" https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/environments?search_query.q=name:dev
{
  "environments": [
    {
      "name": "developmen1t",
      "description": ""
    },
    {
      "name": "dev3",
      "description": ""
    },
   ...
  ],
  "page": 0,
  "total": 5,
}

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

📷 Screenshots, if applicable

Signed-off-by: Vivek Singh [email protected]

Aha! Link: https://chef.aha.io/epics/SH-E-392

netlify · 2021-01-28T04:50:47Z

Deploy preview for chef-automate ready!

Built with commit 60bfc86

https://deploy-preview-4659--chef-automate.netlify.app

kvivek1115 · 2021-01-28T12:43:29Z

components/infra-proxy-service/server/environments.go

+// Environment represents the search based deserialized Environment type
+type Environment struct {
+	Name        string `json:"name"`
+	Description string `json:"description"`


Only fetching the desired data using search API

kvivek1115 · 2021-01-28T12:44:27Z

components/infra-proxy-service/server/environments.go

+		query = chef.SearchQuery{
+			Index: "environment",
+			Query: "*:*",
+			Start: 0,
+			Rows:  inc,


Default query string to fetch the first time, other records get appended.

kvivek1115 · 2021-01-28T12:45:02Z

components/infra-proxy-service/server/environments.go

-	sort.Slice(cl, func(i, j int) bool {
-		return cl[i].Name < cl[j].Name
-	})
-


No need to sort here

jaym · 2021-01-29T14:39:53Z

components/infra-proxy-service/server/environments.go

+				return result, ParseAPIError(err)
+			}
+
+			defer res1.Body.Close() // nolint: errcheck


defer in loop can be problematic. Same comment made on your other pr

remove it entirely, not required anymore

vivekshankar1 · 2021-01-29T14:56:17Z

components/infra-proxy-service/server/environments.go

 	}, nil
 }

+// SearchEnvironments gets environment list from Chef Infra Server search API.
+func (c *ChefClient) SearchEnvironments(searchQuery *request.SearchQuery) (EnvironmentResult, error) {


As we are doing the same functionality across environment, client, data-bag. Can we reuse this method ?

Yes, it would be some reusability left with these PR but for that, we some need something into master, I have planned for that but not now thanks

refactored the code, now using c.client.Seach.Query instead of raw query. thanks!

alexpop · 2021-01-29T16:21:51Z

Hey @vsingh-msys is it possible to add some integration tests for these APIs?

msorens · 2021-01-30T01:02:08Z

api/external/infra_proxy/request/environments.proto

@@ -5,11 +5,22 @@ option go_package = "github.com/chef/automate/api/external/infra_proxy/request";

 import "google/protobuf/struct.proto";

+message SearchQuery {
+	// The search query used to identify a list of items.


This needs a lot more information on what constitutes a query.
Your PR preamble gives some...
"... has to be in the format :, name:* or name:chef-env-* etc."
but that is insufficient. What do you mean by "etc." ? Must there always be a colon? Can there be more than one colon. If a wildcard can exist in *:* then surely we could also do "*:chef-env", but there is no example of that.
It is OK to have examples, but also need to explain the requirements.
What I see from this small set of examples:

Format must be x:y where x is ?? and y is ??.

x may be a field name (what fields??) or a wildcard.

y may be a complete value (what values??), a value prefix ending with a wildcard, or a wildcard by itself.
That is just my inference; please adjust this to accurately reflect the requirements.
(This applies to several PRs and several proto files in each PR).

So here in the environment list, currently targeting for searching with the name only.

Have taken reference from chef-manage so if nothing is provided for search use *:* for wildcard

otherwise use name:SEARCH_PATTERN like example.

For more context on search pattern chef search support key:search_pattern ref: https://docs.chef.io/chef_search/#query-syntax

where key is a field name that is found in the JSON description of an indexable object on the Chef Infra Server (a role, node, client, environment, or data bag) and search_pattern defines what will be searched for, using one of the following search patterns: exact, wildcard, range, or fuzzy matching. Both key and search_pattern are case-sensitive; key has limited support for multiple character wildcard matching using an asterisk ("*") (and as long as it is not the first character).

msorens · 2021-01-30T01:03:48Z

api/external/infra_proxy/request/environments.proto

+message SearchQuery {
+	// The search query used to identify a list of items.
+	string q = 1;
+	// The number of result pages to return.


The PR preamble disagrees, stating start is the "offset of the records" which I think is correct and L11 here is incorrect. But what units is offset? I am guessing that is "pages" where a page is defined by the rows property?
(This applies to several PRs and several proto files in each PR).

ohh yeah, will match and updated it accordingly.

Also, I tried to match parity with chef server API to named the exact same. ref https://docs.chef.io/server/api_chef_server/#get-47

Automate uses most of the places per_page, page variables for pagination. do we need to rename it?

Yes, I missed that; good catch. Looking over the existing ones in the rest of the code base, the description for page is wrong on most of them.
Let's go with this, please:

// Starting page for the results. int32 page = 23; // Number of results on each page. int32 per_page = 24;

Thanks for the renames, but you missed my last comment above: the comment on page is wrong in a couple places where you have copied it. Needs to be as shown above, "Starting page for the results."

Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 added the automate-infra-proxy label Jan 28, 2021

kvivek1115 added this to the Automate 2021 Q1 SP1 milestone Jan 28, 2021

kvivek1115 requested a review from a team as a code owner January 28, 2021 04:50

kvivek1115 self-assigned this Jan 28, 2021

kvivek1115 commented Jan 28, 2021

View reviewed changes

jaym reviewed Jan 29, 2021

View reviewed changes

vivekshankar1 self-requested a review January 29, 2021 14:51

vivekshankar1 reviewed Jan 29, 2021

View reviewed changes

msorens reviewed Jan 30, 2021

View reviewed changes

kvivek1115 marked this pull request as draft February 2, 2021 11:57

kvivek1115 modified the milestones: Automate 2021 Q1 SP1, Automate 2021 Q1 SP2 Feb 5, 2021

kvivek1115 force-pushed the VSingh/infra-proxy-service-environment-search-api branch from 1e6989f to 42e8ce3 Compare February 11, 2021 13:20

kvivek1115 marked this pull request as ready for review February 11, 2021 15:50

Vivek Singh added 7 commits February 11, 2021 21:36

search environments using search API

f18f2d3

Signed-off-by: Vivek Singh <[email protected]>

default per page records

89c5931

Signed-off-by: Vivek Singh <[email protected]>

refactored the code and rename search params with perpage & page

b7c61b6

Signed-off-by: Vivek Singh <[email protected]>

proto comments updates

f733d70

Signed-off-by: Vivek Singh <[email protected]>

Added page to response object and environments test cases

ef19bff

Signed-off-by: Vivek Singh <[email protected]>

proto changes

a019a9c

Signed-off-by: Vivek Singh <[email protected]>

reuse common protobuff

60bfc86

Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 force-pushed the VSingh/infra-proxy-service-environment-search-api branch from 591ed6c to 60bfc86 Compare February 11, 2021 16:16

msorens approved these changes Feb 12, 2021

View reviewed changes

kvivek1115 requested a review from vivekshankar1 February 12, 2021 04:29

kalroy merged commit e640b91 into master Feb 12, 2021

kalroy deleted the VSingh/infra-proxy-service-environment-search-api branch February 12, 2021 05:28

kvivek1115 added the acceptance: verified label Feb 18, 2021

AnkurMundhra added Epic and removed automate-infra-proxy acceptance: verified labels Aug 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[infra-proxy-service] Environments list using search API with pagination #4659

[infra-proxy-service] Environments list using search API with pagination #4659

kvivek1115 commented Jan 28, 2021 •

edited by AnkurMundhra

Loading

netlify bot commented Jan 28, 2021 •

edited

Loading

kvivek1115 Jan 28, 2021

kvivek1115 Jan 28, 2021

kvivek1115 Jan 28, 2021

jaym Jan 29, 2021

kvivek1115 Feb 11, 2021

vivekshankar1 Jan 29, 2021

kvivek1115 Jan 29, 2021

kvivek1115 Feb 11, 2021

alexpop commented Jan 29, 2021

msorens Jan 30, 2021

kvivek1115 Feb 2, 2021

msorens Jan 30, 2021

kvivek1115 Feb 2, 2021

msorens Feb 2, 2021

msorens Feb 9, 2021

[infra-proxy-service] Environments list using search API with pagination #4659

[infra-proxy-service] Environments list using search API with pagination #4659

Conversation

kvivek1115 commented Jan 28, 2021 • edited by AnkurMundhra Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

netlify bot commented Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexpop commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvivek1115 commented Jan 28, 2021 •

edited by AnkurMundhra

Loading

netlify bot commented Jan 28, 2021 •

edited

Loading