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

response actual resource list from search api #3312

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

tedli
Copy link
Contributor

@tedli tedli commented Mar 22, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

response actual resource list, then client code like client-go can decode response body into object corresponding to pre defined api type.

Which issue(s) this PR fixes:
Fixes #3311

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-search`: response actual resource list from search API.

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 22, 2023
@karmada-bot karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 22, 2023
@tedli tedli changed the title Kindlist response actual resource list from search api Mar 22, 2023
@XiShanYongYe-Chang
Copy link
Member

/assign

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Thanks~
Can you help post your test result?

pkg/registry/search/storage/cache.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2023

Codecov Report

Merging #3312 (97872b4) into master (6647508) will increase coverage by 10.11%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           master    #3312       +/-   ##
===========================================
+ Coverage   38.98%   49.10%   +10.11%     
===========================================
  Files         207      209        +2     
  Lines       19365    18763      -602     
===========================================
+ Hits         7549     9213     +1664     
+ Misses      11360     9049     -2311     
- Partials      456      501       +45     
Flag Coverage Δ
unittests 49.10% <0.00%> (+10.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/registry/search/storage/cache.go 0.00% <0.00%> (ø)
pkg/registry/search/storage/search.go 0.00% <0.00%> (ø)

... and 68 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tedli
Copy link
Contributor Author

tedli commented Mar 22, 2023

Thanks~ Can you help post your test result?

You mean unit test? Or something like, I make a POC demo, to make sure the idea of this pr works as expect?

@XiShanYongYe-Chang
Copy link
Member

You mean unit test? Or something like, I make a POC demo, to make sure the idea of this pr works as expect?

Sorry, I didn't make it clear, just referring to a simple access and return result.

@tedli
Copy link
Contributor Author

tedli commented Mar 22, 2023

Hi @XiShanYongYe-Chang
asciicast

@XiShanYongYe-Chang
Copy link
Member

Thanks~
/lgtm
Can you help update the release note like that:

`karmada-search`: response actual resource list from search api

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2023
@XiShanYongYe-Chang
Copy link
Member

/cc @RainbowMango
Can you help to run the CI?

@tedli
Copy link
Contributor Author

tedli commented Mar 22, 2023

podlist.mp4
type response[T runtime.Object] struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`
	Items             []T
}

func listPod(client kubernetes.Interface) {
	// var raw []byte
	var raw runtime.Object
	var err error
	if raw, err = client.CoreV1().RESTClient().Get().RequestURI(
		fmt.Sprintf("/apis/search.karmada.io/v1alpha1/search/cache/api/v1/namespaces/%s/pods", "lizhen6")).
		//DoRaw(context.Background()); err != nil {
		Do(context.Background()).Get(); err != nil {
		panic(err)
	}
	//var pods response[*corev1.Pod]
	//if err = json.Unmarshal(raw, &pods); err != nil {
	//	panic(err)
	//}
	pods, ok := raw.(*corev1.PodList)
	if !ok {
		panic("not pod list")
	}
	fmt.Printf("===== List Pod =====\n")
	for _, item := range pods.Items {
		fmt.Printf("Kind: %s, Name: %s, Namespace: %s, Annotations[\"resource.karmada.io/cached-from-cluster\"]: %s\n",
			kind(item), item.Name, item.Namespace, item.Annotations["resource.karmada.io/cached-from-cluster"])
	}
}

func kind(obj interface{}) string {
	return reflect.Indirect(reflect.ValueOf(obj)).Type().Name()
}

@XiShanYongYe-Chang
Copy link
Member

@tedli, thanks very much, the test you provided was very detailed

@tedli
Copy link
Contributor Author

tedli commented Mar 22, 2023

Thanks~ /lgtm Can you help update the release note like that:

`karmada-search`: response actual resource list from search api

Do you mean add a CHANGELOG-1.6.md at docs\CHANGELOG and mention this in it? If so would the next release be 1.6 or other lucky number?

@XiShanYongYe-Chang
Copy link
Member

Do you mean add a CHANGELOG-1.6.md at docs\CHANGELOG and mention this in it? If so would the next release be 1.6 or other lucky number?

I'm referring to this place:
image

@XiShanYongYe-Chang
Copy link
Member

An occasional error occurred in the CLI test, not related to the current modification.

Hi @tedli, can you squash your two commit into one commit?

Signed-off-by: lizhen6 <lizhen6@360.cn>
@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2023
@tedli
Copy link
Contributor Author

tedli commented Mar 22, 2023

An occasional error occurred in the CLI test, not related to the current modification.

Hi @tedli, can you squash your two commit into one commit?

Done.

@XiShanYongYe-Chang
Copy link
Member

Thanks~
/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2023
@XiShanYongYe-Chang
Copy link
Member

/assign @RainbowMango

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango, XiShanYongYe-Chang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2023
@karmada-bot karmada-bot merged commit 861bc7a into karmada-io:master Mar 23, 2023
@RainbowMango
Copy link
Member

@tedli I just noticed that GitHub can't recognize you by your email in the commit(author part).
If you want GitHub to count your contribution, please add your email to GitHub Emails, note that doesn't mean you have to public your email, it still can be private.

@tedli
Copy link
Contributor Author

tedli commented Mar 27, 2023

@tedli I just noticed that GitHub can't recognize you by your email in the commit(author part). If you want GitHub to count your contribution, please add your email to GitHub Emails, note that doesn't mean you have to public your email, it still can be private.

Thanks. I've updated, adding the email in git commit to github.

@RainbowMango
Copy link
Member

Thanks. OK now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

response actual resource list from search cache api
5 participants