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

Fix lint issues in target allocator #1090

Merged
merged 3 commits into from
Oct 3, 2022

Conversation

kristinapathak
Copy link
Contributor

I noticed that the target allocator files don't have a copyright at the top. Given how uniform it was, I'm not sure if this was intentional, but I added them here in case they are missing.

@kristinapathak kristinapathak requested a review from a team September 13, 2022 01:33
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Should this repo include a license-check target that ensures these are present? We do this in the Go SDK.

@kristinapathak
Copy link
Contributor Author

@Aneurysm9, I added the target you recommended - it runs as a part of basic checks:
https://github.com/open-telemetry/opentelemetry-operator/actions/runs/3046982755/jobs/4910456529

.ci/create-release-github.sh Outdated Show resolved Hide resolved
@pavolloffay pavolloffay changed the title add copyright to top of target allocator files Add copyright headers to target allocator Sep 15, 2022
@kristinapathak
Copy link
Contributor Author

Golang-ci is not running on target allocator code. I will be working on getting golang-ci to run on all go modules in the repo and fixing other lint issues in the target allocator.

@kristinapathak
Copy link
Contributor Author

kristinapathak commented Sep 19, 2022

@pavolloffay, how was the configuration in .golangci.yaml decided? Other opentelemetry repos have golangci configured in a different way than this one, most notably for govet:
https://github.com/open-telemetry/opentelemetry-go-build-tools/blob/29a46c89ab69347be7d6f2c7f4f94135cb4d0d4f/.golangci.yml#L58
https://github.com/open-telemetry/opentelemetry-collector/blob/b1329c94b3b31b9ffa4091ab5066d6f091d71f41/.golangci.yml#L58
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/d2490af3003387cc4b1baa5b55f7d748303d7a4b/.golangci.yml#L56

There are many govet fieldalignment issues that look like this:

allocation/consistent_hashing.go:40:33: fieldalignment: struct with 64 pointer bytes could be 40 (govet)
type consistentHashingAllocator struct {

I'm not sure the best path forward to fix these specific fieldalignment issues.

@kristinapathak kristinapathak changed the title Add copyright headers to target allocator Fix lint issues in target allocator Sep 20, 2022
@pavolloffay
Copy link
Member

@pavolloffay, how was the configuration in .golangci.yaml decided?

I am not sure, probably back then I was not working in this repo. You could browse the PR history.

I am fine with disabling fieldalignment and aligning the golang-ci to other projects in the organization (if they use the same config).

@kristinapathak
Copy link
Contributor Author

@pavolloffay, bringing the golangci.yaml into alignment with the golangci configs in opentelemetry-collector and opentelemetry-collector-contrib means even more lint fixes. I would rather disable fieldalignment in this PR, merge this, and then work on the next lint changes in a new PR. Is that okay with you?

@pavolloffay
Copy link
Member

Is that okay with you?

Absoulutelly.

I am not sure why CI job Code standards (lining) didn't run

@pavolloffay
Copy link
Member

maybe try to squash the commits and rebased the PR

squash of all current commits on this branch
@kristinapathak
Copy link
Contributor Author

Further lint work: #1122

@kristinapathak
Copy link
Contributor Author

@pavolloffay, I think there is an issue from Code standards (linting) being in a matrix now:

Continuous Integration / Code standards (linting) (.) (pull_request) 
Continuous Integration / Code standards (linting) (./cmd/otel-allocator) (pull_request) 

@@ -0,0 +1,13 @@
Copyright The OpenTelemetry Authors
Copy link
Member

Choose a reason for hiding this comment

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

could we remove this as well and reuse the header defined in the project root?

@kristinapathak kristinapathak deleted the copyright branch October 3, 2022 16:26
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* fix lint issues in target allocator files

squash of all current commits on this branch

* remove addtl golangci config

* remove version.txt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants