Add notimestamp linter #105

Karthik-K-N · 2025-06-25T18:07:12Z

Adds a new notimestamp linter that checks for any fields in struct having timestamp string or substring.

Fixes: #26

k8s-ci-robot · 2025-06-25T18:07:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Karthik-K-N
Once this PR has been reviewed and has the lgtm label, please assign jpbetz for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

everettraven

Unlike the nophase linter, this linter has a suggested replacement for the substring timestamp (time).

At the very least, we should ensure that we include that replacement suggestion in the report message.

everettraven · 2025-06-25T18:53:00Z

pkg/analysis/notimestamp/analyzer.go

+		// First check if the struct field name contains 'timestamp'
+		if strings.Contains(strings.ToLower(fieldName), "timestamp") {
+			pass.Reportf(field.Pos(),
+				"field %s: fields with timestamp substring should be avoided",


We should probably include in the message what alternative should be used in place of this. I believe the appropriate replacement here would be time.

Seeing as how this linter has a proposed replacement, should we also implement the suggested fix for this so that if someone were to run lint --fix that it would automatically apply the suggestion?

hey, A quick question, whats the right way of debugging or to get to know the value of some variable, If I try

fmt.Println(fieldName)

I dont see anything getting printed on the console

How are you trying to debug it? Running the actual tool or via the tests?

If it is through the tests, you'd want to tell it to be verbose using the -v flag with go test.

oh ok, I will give a try, Thanks, I was trying with tool

./bin/golangci-kube-api-linter run kube-api-linter/pkg/analysis/notimestamp/testdata/src/a/a.go --fix

pkg/analysis/notimestamp/analyzer.go

Karthik-K-N · 2025-06-30T13:54:16Z

Updated the PR to address review comments, Couple of pending questions

Currently I need to run

./bin/golangci-kube-api-linter run /kube-api-linter/pkg/analysis/notimestamp/testdata/src/a/a.go --fix

twice to fix the names, Once for fixing field names and second time is for fixing json tags. Not sure thats ideal.

GIven that we are updating the json tags and field comments are starting with json tags, Should we fix that comment also to use the latest tag or can we depend on the commentStart linter to fix it, by running it again.

JoelSpeed · 2025-06-30T14:19:30Z

docs/linters.md

+
+The `notimestamp` linter checks that the fields in the API types don't contain a 'Timestamp', or any field which contains 'Timestamp' as a substring, e.g CreateTimestamp.


Maybe?

Suggested change

The `notimestamp` linter checks that the fields in the API types don't contain a 'Timestamp', or any field which contains 'Timestamp' as a substring, e.g CreateTimestamp.

The `notimestamp` linter checks that the fields in the API are not named with the word 'Timestamp'.

What is the reason for this from the API conventions, might be good to help explain that context here a little?

Is there any other place apart from https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#naming-conventions, To look for reasons for avoiding it.

I would generally try to use git blame to work out the PR that introduced the rule, and see if there is additional context there. If not, perhaps #api-reviews on the Kuberentes slack might be able to help

sure thanks, I will check

JoelSpeed · 2025-06-30T14:24:26Z

pkg/analysis/notimestamp/analyzer.go

+		return
+	}
+
+	var suggestedFixes []analysis.SuggestedFix


Fixes should be explained in the linters documentation, see how others have done that

JoelSpeed · 2025-07-09T15:17:54Z

docs/linters.md

+
+The `notimestamp` linter checks that the fields in the API are not named with the word 'Timestamp'.
+
+The name of a field that specifies the time at which something occurs should be called `somethingTime`.Its recommended not use stamp (e.g., creationTimestamp).


Suggested change

The name of a field that specifies the time at which something occurs should be called `somethingTime`.Its recommended not use stamp (e.g., creationTimestamp).

The name of a field that specifies the time at which something occurs should be called `somethingTime`. It is recommended not use stamp (e.g., creationTimestamp).

JoelSpeed · 2025-07-09T15:20:08Z

pkg/analysis/notimestamp/initializer.go

+// Initializer returns the AnalyzerInitializer for this
+// Analyzer so that it can be added to the registry.
+func Initializer() initializer {
+	return initializer{}
+}
+
+// intializer implements the AnalyzerInitializer interface.
+type initializer struct{}
+
+// Name returns the name of the Analyzer.
+func (initializer) Name() string {
+	return name
+}
+
+// Init returns the intialized Analyzer.
+func (initializer) Init(cfg config.LintersConfig) (*analysis.Analyzer, error) {
+	return Analyzer, nil
+}
+
+// Default determines whether this Analyzer is on by default, or not.
+func (initializer) Default() bool {
+	return true
+}


A recent refactor has changed the way functions init, can you please rebase and look at another, non-configurable linter to see how this should look now.

commentstart is probably a good example to crib from

sure will do that

JoelSpeed · 2025-07-09T15:23:07Z

pkg/analysis/registry.go

@@ -86,6 +87,7 @@ func NewRegistry() Registry {
 			nofloats.Initializer(),
 			nomaps.Initializer(),
 			nophase.Initializer(),
+			notimestamp.Initializer(),


This will conflict when you rebase, we don't maintain this list anymore as each linter now self registers by blank importing in a package called registration

JoelSpeed · 2025-07-09T15:25:12Z

pkg/analysis/notimestamp/analyzer.go

+	fieldReplacementName := timeStampRegEx.ReplaceAllString(fieldName, "Time")
+	if fieldReplacementName != fieldName {
+		suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{
+			Message: "replace field with Time",


I think these normally have to be unique, have you tried this against a file that has multiple failures using a full golangci-lint run?

k8s-ci-robot · 2025-07-09T19:12:13Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 25, 2025

k8s-ci-robot requested review from everettraven and jpbetz June 25, 2025 18:07

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2025

everettraven reviewed Jun 25, 2025

View reviewed changes

everettraven mentioned this pull request Jun 25, 2025

poc: generic naming convention linter #106

Open

JoelSpeed reviewed Jun 26, 2025

View reviewed changes

pkg/analysis/notimestamp/analyzer.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2025

Add notimestamp linter

257922b

Karthik-K-N force-pushed the add-notimestamp branch from 9957ad0 to f751830 Compare June 30, 2025 13:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2025

Karthik-K-N force-pushed the add-notimestamp branch from f751830 to e462791 Compare June 30, 2025 13:58

JoelSpeed reviewed Jun 30, 2025

View reviewed changes

Address reveiw comments

627e267

Karthik-K-N force-pushed the add-notimestamp branch from e462791 to 627e267 Compare June 30, 2025 15:06

sbueringer mentioned this pull request Jul 7, 2025

Umbrella issue: API changes kubernetes-sigs/cluster-api#10852

Open

91 tasks

JoelSpeed reviewed Jul 9, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2025


		The `notimestamp` linter checks that the fields in the API types don't contain a 'Timestamp', or any field which contains 'Timestamp' as a substring, e.g CreateTimestamp.


		The `notimestamp` linter checks that the fields in the API are not named with the word 'Timestamp'.

		The name of a field that specifies the time at which something occurs should be called `somethingTime`.Its recommended not use stamp (e.g., creationTimestamp).

Add notimestamp linter #105

Are you sure you want to change the base?

Add notimestamp linter #105

Conversation

Karthik-K-N commented Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 25, 2025

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Karthik-K-N commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 9, 2025

Uh oh!

Uh oh!