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.

Sign up for GitHub

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

Jump to bottom

Add "Deprecated" annotation to Pending #5224

Closed
wants to merge 1 commit into from
Closed

Add "Deprecated" annotation to Pending #5224

wants to merge 1 commit into from

Conversation

shi0rik0
Copy link
Contributor

Add a "Deprecated" annotation to Pending, so that the IDE will show hints that this phase is deprecated.

luolanzone
luolanzone reviewed Jul 10, 2023
View reviewed changes
pkg/apis/crd/v1alpha1/types.go
@@ -23,7 +23,7 @@ import (
type TraceflowPhase string

const (
// Pending is not used anymore
// Deprecated: Pending is not used anymore
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's reasonable to refine comment for IDE behavior since different IDEs may have different requirements. @antoninbas @tnqn any comments?

Copy link
Contributor

@luolanzone luolanzone Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If it's necessary to show deprecated info, I may include the change in PR #5108.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very common convention. Please see https://zchee.github.io/golang-wiki/Deprecated.

"Some tools will warn on use of deprecated identifiers and their docs are hidden on pkg.go.dev."

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine to me.
@luolanzone we can merge this PR as is, and you can remove Pending altogether for v1beta1/types.go in your PR

antoninbas
antoninbas previously approved these changes Jul 10, 2023
View reviewed changes
@antoninbas
Copy link
Contributor

/skip-all

@shi0rik0 shi0rik0 requested a review from luolanzone July 11, 2023 12:06
@antoninbas
Copy link
Contributor

antoninbas commented Jul 11, 2023 •
edited
Loading

@shi0rik0

I can't actually merge this because the linters are now complaining:

2023-07-10T18:22:16.3458440Z ===> Installing Golangci-lint <===
2023-07-10T18:22:16.4436261Z golangci/golangci-lint info checking GitHub for tag 'v1.52.2'
2023-07-10T18:22:16.5419976Z golangci/golangci-lint info found version: 1.52.2 for v1.52.2/linux/amd64
2023-07-10T18:22:17.2732294Z golangci/golangci-lint info installed /home/runner/work/antrea/antrea/.golangci-bin/v1.52.2/golangci-lint
2023-07-10T18:22:17.2827933Z ===> Running golangci (linux) <===
2023-07-10T18:28:32.6235651Z ##[error]pkg/controller/traceflow/controller.go:240:11: SA1019: crdv1alpha1.Pending is deprecated: Pending is not used anymore (staticcheck)
2023-07-10T18:28:32.6243904Z 	case "", crdv1alpha1.Pending:
2023-07-10T18:28:32.6244200Z 	         ^
2023-07-10T18:28:32.7067593Z make: *** [Makefile:291: golangci] Error 1

IMO, it is ok to remove crdv1alpha1.Pending from the switch for the following reasons:

  • The .Status.Phase field is populated by Antrea and the Pending value has been deprecated for at least 2 years
  • Traceflow objects are typically ephemeral (short-lived)

cc @gran-vmv @luolanzone

@antoninbas
Copy link
Contributor

Of course, we could also skip this PR altogether (or "merge" it with @luolanzone's PR) or add a temporary ignore directive for SA1019

@shi0rik0
Copy link
Contributor Author

Of course, we could also skip this PR altogether (or "merge" it with @luolanzone's PR) or add a temporary ignore directive for SA1019

I've added a ignore directive for SA1019.

@shi0rik0
Copy link
Contributor Author

@antoninbas

@shi0rik0 shi0rik0 requested a review from antoninbas July 12, 2023 08:29
@antoninbas
Copy link
Contributor

/skip-all

antoninbas
antoninbas reviewed Jul 12, 2023
View reviewed changes
pkg/controller/traceflow/controller.go Outdated
Comment on lines 240 to 242
//lint:ignore SA1019 Allow existing use.
case "", crdv1alpha1.Pending:
Copy link
Contributor

Choose a reason for hiding this comment

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

@luolanzone you can remove the lint directive and the Pending reference in your Traceflow graduation PR

antoninbas
antoninbas previously approved these changes Jul 12, 2023
View reviewed changes
@antoninbas
Copy link
Contributor

Still cannot merge as CI is failing

@shi0rik0
Add "Deprecated" annotation to Pending
87eb11c 
Add a "Deprecated" annotation to Pending, so that the IDE will show hints that
this phase is deprecated.

Signed-off-by: shi0rik0 <[email protected]>
@shi0rik0
Copy link
Contributor Author

Still cannot merge as CI is failing

I've fixed it. This is because golangci-lint binary only uses staticcheck rules, but not its binary. So golangci-lint and staticcheck have different ignore hint formats.

golangci/golangci-lint#741

@shi0rik0 shi0rik0 requested a review from antoninbas July 26, 2023 14:56
@shi0rik0
Copy link
Contributor Author

@antoninbas

@antoninbas
Copy link
Contributor

sorry for the delay. With the Traceflow promotion PR merged, the change in pkg/controller/traceflow/controller.go is no longer applicable

@shi0rik0 shi0rik0 closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luolanzone luolanzone Awaiting requested review from luolanzone

@antoninbas antoninbas Awaiting requested review from antoninbas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shi0rik0 @antoninbas @luolanzone