Skip to content

[v17] Fix kube discovery service poll_interval value not set correctly#61792

Merged
kshi36 merged 6 commits intobranch/v17from
kevin/backport-60982-branch/v17
Dec 1, 2025
Merged

[v17] Fix kube discovery service poll_interval value not set correctly#61792
kshi36 merged 6 commits intobranch/v17from
kevin/backport-60982-branch/v17

Conversation

@kshi36
Copy link
Copy Markdown
Contributor

@kshi36 kshi36 commented Nov 25, 2025 •
edited
Loading

Backport #60982 to branch/v17

changelog: Fixed bug where Kubernetes App Discovery poll_interval is not set correctly

@kshi36
Fix kube discovery service poll_interval value not set correctly (#…
e551533 
…60982)

* Fix kube discovery service poll interval

* Add unit test, pass down clocks in discovery configs

* Leverage synctest, pass down clocks properly

* Refactor
@kshi36
Copy link
Copy Markdown
Contributor Author

kshi36 commented Nov 25, 2025

Since branch/v17 does not include discovery_eks_test.go, I omitted changes to test files (discovery_test.go).

@kshi36 kshi36 marked this pull request as ready for review November 25, 2025 23:00
@rosstimothy
Copy link
Copy Markdown
Contributor

rosstimothy commented Nov 26, 2025

Since branch/v17 does not include discovery_eks_test.go, I omitted changes to test files (discovery_test.go).

Can we add discovery_eks_test.go with your new test and any mocks required instead of omitting the tests entirely?

r0mant
r0mant approved these changes Nov 26, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm once Tim's comment is addressed

kshi36
kshi36 commented Nov 26, 2025
View reviewed changes
lib/srv/discovery/discovery_eks_test.go
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

synctest.Test(t, func(t *testing.T) {
Copy link
Copy Markdown
Contributor Author

@kshi36 kshi36 Nov 26, 2025

Choose a reason for hiding this comment

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

Utilizes testutils.synctest, same as v18

kshi36
kshi36 commented Nov 26, 2025
View reviewed changes
lib/srv/discovery/discovery_eks_test.go
Comment on lines +112 to +114
cloudClients: &cloud.TestCloudClients{
STS: &mocks.STSMock{},
EKS: &mocks.EKSMock{
Copy link
Copy Markdown
Contributor Author

@kshi36 kshi36 Nov 26, 2025

Choose a reason for hiding this comment

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

Refactored to use cloud.TestCloudClients to match usage in v17 tests

@kshi36
Copy link
Copy Markdown
Contributor Author

kshi36 commented Nov 26, 2025

Can we add discovery_eks_test.go with your new test and any mocks required instead of omitting the tests entirely?

I added discovery_eks_test.go by migrating impacted test cases from discovery_test.go, like in #56427, with refactoring to match v17 APIs. LMK if this is acceptable.

rosstimothy
rosstimothy approved these changes Dec 1, 2025
View reviewed changes
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from bernardjkim December 1, 2025 09:51
@kshi36 kshi36 mentioned this pull request Dec 1, 2025
Merged
@kshi36 kshi36 added this pull request to the merge queue Dec 1, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 1, 2025
@kshi36 kshi36 added this pull request to the merge queue Dec 1, 2025
Merged via the queue into branch/v17 with commit b63a133 Dec 1, 2025
37 of 38 checks passed
@kshi36 kshi36 deleted the kevin/backport-60982-branch/v17 branch December 1, 2025 18:25
@doggydogworld doggydogworld mentioned this pull request Dec 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@r0mant r0mant r0mant approved these changes

@rosstimothy rosstimothy rosstimothy approved these changes

Assignees

No one assigned

Labels

backport discovery size/sm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kshi36 @rosstimothy @r0mant