-
Notifications
You must be signed in to change notification settings - Fork 429
Enabled tagliatelle and added relevant exclusions for it #7442
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
base: main
Are you sure you want to change the base?
Enabled tagliatelle and added relevant exclusions for it #7442
Conversation
.golangci.yml
Outdated
| rules: | ||
| json: goCamel | ||
| yaml: goCamel | ||
| ignored-fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for each field, could you add as a comment the actual tag name vs the expected one, for reference and to help with review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an incorrect version of the file. I am adding the new one right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you let me know if there any further issues?
|
I am currently waiting for an tagliatelle issue to be resolved. Issue |
ca75cf1 to
bfead10
Compare
|
I have added the expected names for reference. I still could not get the custom initialisms to work. |
bfead10 to
e237c46
Compare
.golangci.yml
Outdated
| - AppliedPolicies # policies : appliedPolicies | ||
| - AppProtocol # app_proto : appProtocol | ||
| - BGPPolicyName # name' want 'bgpPolicyName | ||
| - BrName #ovsBinding : brName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in a test file, can we exclude test files?
.golangci.yml
Outdated
| - ClusterNetworkPolicy # clusternetworkpolicy : clusterNetworkPolicy | ||
| - ContentLength # length : contentLength | ||
| - ContentType # http_content_type : contentType | ||
| # - AppliedPolicies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a duplicate that can be removed?
.golangci.yml
Outdated
| rules: | ||
| json: goCamel | ||
| yaml: goCamel | ||
| ignored-fields: # Current: Expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know if the "scope" for each ignored field can be limited to a single type / file? I don't think we want exceptions to apply "globally"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to see if it is possible to do this.
This reverts commit ca75cf1.
e237c46 to
c40312e
Compare
|
I addressed the issues you previously mentioned @antoninbas. |
| - pkg: test | ||
| ignore: true | ||
| #Format for ignored-fields comments - current : expected | ||
| - pkg: pkg/ovs/ovsconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be ignored as well. We don't control the tags, they are based on the OVSDB schema.
|
|
||
| - pkg: pkg/flowaggregator/apis | ||
| ignored-fields: | ||
| - WithIPFIXExporter # withIPFIXExporter : withIpfixExporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add IPFIX as an initialism, or did you say it was not working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the following snippet but it did not work out.
Snippet:
Extended-rules:
yaml:
case: goCamel
extra-initialisms: true
initialism-overrides:
IFPIX: true
IPAM: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also asked the folks at tagliatelle, to see if it's a error from my end or there is a problem with the linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a response from their end: ldez/tagliatelle#30 (comment)
I will try resolving it from my end and if it is not possible, raise an issue for tagliatelle.
| - ClientCAFile # clientCAFile : clientCaFile | ||
| - ClusterCIDRs # clusterCIDRs : clusterCidRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CA & CIDR should also be added as initialisms
| - NodeResults # results : nodeResults | ||
| - TransportHeader # tranportHeader : transportHeader | ||
|
|
||
| - pkg: pkg/agent/flowexporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whole package be ignored as well, we don't control these names
| - pkg: pkg/agent/util | ||
| ignored-fields: | ||
| - Extensions # Extensions : extensions | ||
| - ExtensionID # Id : extensionID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, I don't think we control or can change these tags
BTW, it may be better to add //nolint:tagliatelle on struct definitions for which we don't have control over the tags. Can you check if it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify a bit about //nolint:tagliatelle and where to place it? Also are agent and ovs the only packages not in our control?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not get the extra-initialisms to work for Antrea. I tried the same yaml config on small dummy programs and it worked. So I am trying to figure out what's causing this issue.
Added tagliatelle in golangci-lint as per requirements mentioned in #7344