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

opt(br): resolve TODOs & add IT cases #6100

Merged
merged 24 commits into from
Feb 28, 2025

Conversation

ideascf
Copy link
Contributor

@ideascf ideascf commented Feb 28, 2025

What problem does this PR solve?

This PR aims to port IT cases from operator v1 to v2 and also resolve the remaining TODOs.

What is changed and how does it work?

  • Port IT cases from v1 to v2, and revive some skipped cases.
  • Support retry for Job and also integrate it for Backup.
  • Unify the log lib to sigs.k8s.io/controller-runtime/pkg/log and log the reconcilerID in each log.
  • Pass the ctx to every needed functions.
  • Fix TrackManager is created multiple times and cause goroutine leak.
  • Some script optimization for easily locally run E2E cases.

Code changes

  • Has Go code change
  • Has CI related scripts change

Tests

  • Unit test
  • E2E test
  • Manual test
  • No code

Side effects

  • Breaking backward compatibility
  • Other side effects:

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release Notes

Please refer to Release Notes Language Style Guide before writing the release note.


Copy link

/run-pull-e2e-kind-v2

@github-actions github-actions bot added the v2 for operator v2 label Feb 28, 2025
@ti-chi-bot ti-chi-bot bot added the size/XXL label Feb 28, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 1.32743% with 669 lines in your changes missing coverage. Please review.

Project coverage is 53.31%. Comparing base (7277fa6) to head (fa31e1c).

Additional details and impacted files
@@              Coverage Diff               @@
##           feature/v2    #6100      +/-   ##
==============================================
- Coverage       54.56%   53.31%   -1.25%     
==============================================
  Files             224      225       +1     
  Lines           15440    15818     +378     
==============================================
+ Hits             8425     8434       +9     
- Misses           7015     7384     +369     
Flag Coverage Δ
unittest 53.31% <1.32%> (-1.25%) ⬇️

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

Copy link

/run-pull-e2e-kind-v2

1 similar comment
@fgksgf
Copy link
Member

fgksgf commented Feb 28, 2025

/run-pull-e2e-kind-v2

Comment on lines 61 to 72
- apiGroups:
- "batch"
resources:
- jobs
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
Copy link
Member

Choose a reason for hiding this comment

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

Did you generate this or add this manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. f4d2fc5

Comment on lines 23 to 35
// ForwardOnePort provide a helper to forward only one port
// and return local endpoint
func ForwardOnePort(ctx context.Context, fw PortForwarder, ns, resourceName string, port int) (string, error) {
ports, err := fw.Forward(ctx, ns, resourceName, []int{port})
if err != nil {
return "", err
}
if len(ports) != 1 {
return "", fmt.Errorf("portforward expect only one port, but now %v", len(ports))
}
localPort := int(ports[0].Local)
return "localhost:" + strconv.Itoa(localPort), nil
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we already have functions like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. 0edbd54

Comment on lines 50 to 54
// PortForwarder represents an interface which can forward local ports to a pod.
type PortForwarder interface {
Forward(ctx context.Context, namespace, resourceName string, remotePorts []int) ([]portforward.ForwardedPort, error)
}

Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. 0edbd54

Comment on lines 127 to 139
// ContainsString checks if a given slice of strings contains the provided string.
// If a modifier func is provided, it is called with the slice item before the comparation.
func ContainsString(slice []string, s string, modifier func(s string) string) bool {
for _, item := range slice {
if item == s {
return true
}
if modifier != nil && modifier(item) == s {
return true
}
}
return false
}
Copy link
Member

Choose a reason for hiding this comment

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

Add UT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. 8d4b22f

Copy link

/run-pull-e2e-kind-v2

Copy link

/run-pull-e2e-kind-v2

@fgksgf
Copy link
Member

fgksgf commented Feb 28, 2025

/run-pull-e2e-kind-v2

@ti-chi-bot ti-chi-bot bot removed the lgtm label Feb 28, 2025
Copy link

/run-pull-e2e-kind-v2

1 similar comment
@fgksgf
Copy link
Member

fgksgf commented Feb 28, 2025

/run-pull-e2e-kind-v2

@fgksgf
Copy link
Member

fgksgf commented Feb 28, 2025

/lgtm

@ti-chi-bot ti-chi-bot bot added the lgtm label Feb 28, 2025
@ti-chi-bot ti-chi-bot bot removed the lgtm label Feb 28, 2025
Copy link

/run-pull-e2e-kind-v2

@ti-chi-bot ti-chi-bot bot added the lgtm label Feb 28, 2025
Copy link
Contributor

ti-chi-bot bot commented Feb 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fgksgf

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

Copy link
Contributor

ti-chi-bot bot commented Feb 28, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-02-28 07:51:00.137678637 +0000 UTC m=+601408.090836903: ☑️ agreed by fgksgf.
  • 2025-02-28 08:27:50.17870169 +0000 UTC m=+603618.131859956: ✖️🔁 reset by ideascf.
  • 2025-02-28 08:35:41.060286696 +0000 UTC m=+604089.013444962: ☑️ agreed by fgksgf.
  • 2025-02-28 10:15:35.874621681 +0000 UTC m=+4649.003541424: ✖️🔁 reset by ideascf.
  • 2025-02-28 13:15:59.668469628 +0000 UTC m=+15472.797389372: ☑️ agreed by fgksgf.

@fgksgf
Copy link
Member

fgksgf commented Feb 28, 2025

/run-pull-e2e-kind-v2

@ti-chi-bot ti-chi-bot bot merged commit 88b5c54 into pingcap:feature/v2 Feb 28, 2025
9 checks passed
@ideascf ideascf deleted the resolve-todo-br branch February 28, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants