Skip to content

Conversation

@ivankatliarchuk
Copy link
Member

@ivankatliarchuk ivankatliarchuk commented Feb 16, 2025

Description

Found the issue in this pull request https://github.com/kubernetes-sigs/external-dns/pull/5097/files#r1957067735

Screenshot 2025-02-16 at 12 18 43
Screenshot 2025-02-16 at 14 44 59

Checklist

  • Unit tests updated
  • End user documentation updated

Signed-off-by: ivan katliarchuk <[email protected]>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 16, 2025
@ivankatliarchuk ivankatliarchuk changed the title chore(format): fix go formatting chore(formatting): fix infected files with correct formatting Feb 16, 2025
@ivankatliarchuk
Copy link
Member Author

ivankatliarchuk commented Feb 16, 2025

Linter fix is here #5085

example fix

linters-settings:
  # Remove or correct the 'maligned' property
  goimports:
    local-prefixes: github.com/kubernetes-sigs/external-dns

run:
  # Remove or correct the 'exclude-files' property
  skip-dirs:
    - vendor
    - internal

@ivankatliarchuk
Copy link
Member Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 16, 2025
with:
go-version-file: go.mod

# https://github.com/golangci/golangci-lint-action?tab=readme-ov-file#verify
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wdyt about adding verify: true in current golangci lint action instead of creating a new one ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed. First impression was, that it does not support both operation, but looks like it does. Validated here https://github.com/golangci/golangci-lint-action/blob/3b4f037d0e94e85d98f9824ef87b2dc32d53fbd5/src/run.ts#L140

# https://github.com/editorconfig/editorconfig-core-go/blob/master/.editorconfig
[{Makefile,go.mod,go.sum,*.go,.gitmodules}]
indent_style = tab
indent_size = 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Current indent_size on all go files in master is 2.
Why would we want to change it to 4 ? It would force to update all codebase and force all PRs to rebase.

side note: there are no gitmodules in this project

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed submodules.
I've added gofmt -l -s -w . to go-lint docs https://pkg.go.dev/cmd/gofmt

I only found this few files that
Screenshot 2025-02-17 at 08 31 56
have issues with intendations, rest of them are fine

Copy link
Member Author

@ivankatliarchuk ivankatliarchuk Feb 17, 2025

Choose a reason for hiding this comment

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

Shell we add gofmt validation to github actions?

Copy link
Member Author

@ivankatliarchuk ivankatliarchuk Feb 17, 2025

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Current indent_size on all go files in master is 2. Why would we want to change it to 4 ? It would force to update all codebase and force all PRs to rebase.

side note: there are no gitmodules in this project

Golang is 4 tabs intendation, which is set across the project.

The proper Go style way to do this is to configure your editor's display of tabs instead.

Is it shows 2 spaces in your editor?

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I checked yesterday, it was displaying 2 spaces.
Today, it display 4 spaces, as you say 😅 .

Shall we add gofmt validation to github actions?

Yes, it sounds good 👍 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice !

@mloiseleur
Copy link
Collaborator

@ivankatliarchuk You'll need to fix the config of golangci-lint and we should be good to go.

@ivankatliarchuk
Copy link
Member Author

Fixed .golangci.yaml file

Copy link
Collaborator

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2025
@k8s-ci-robot k8s-ci-robot merged commit 99b9d0d into kubernetes-sigs:master Feb 18, 2025
13 checks passed
ivankatliarchuk added a commit to gofogo/k8s-sigs-external-dns-fork that referenced this pull request Feb 18, 2025
* master: (97 commits)
  chore(formatting): fix infected files with correct formatting (kubernetes-sigs#5099)
  docs: Fix managed-record-type argument
  docs(proposal): support multiple replicas with leader election (kubernetes-sigs#5051)
  feat(chart): automate helm json schema (kubernetes-sigs#5075)
  docs(proposal): update proposal template, add statuses
  test(aws): introduce first fixture-based (kubernetes-sigs#5092)
  chore(makefile): add helper and document targets
  feat: Updated chart for v1.15.2 release
  chore(makefile): add helper and document targets
  chore(filter-tags): pre-process filter tags
  chore(filter-tags): pre-process filter tags
  chore(filter-tags): pre-process filter tags
  chore(deps): bump the dev-dependencies group across 1 directory with 21 updates
  test(domain-filter): simple filters on domain exclusion (kubernetes-sigs#5064)
  chore(deps): bump nosborn/github-action-markdown-cli
  ci(docs): add markdown linters and editorconfig (kubernetes-sigs#5055)
  Address PR comments
  docs: update and refactor contribution part (kubernetes-sigs#5073)
  fix(chart): update rbac for F5 transportserver source (kubernetes-sigs#5066)
  fix(chart): non-string types on svcaccount annotations (kubernetes-sigs#5067)
  ...
ivankatliarchuk added a commit to gofogo/k8s-sigs-external-dns-fork that referenced this pull request Feb 19, 2025
* master: (31 commits)
  fix(source): debug log on gateway target detection
  Update docs/sources/service.md
  chore(formatting): fix infected files with correct formatting (kubernetes-sigs#5099)
  docs: Fix managed-record-type argument
  Update docs/sources/service.md
  docs(proposal): support multiple replicas with leader election (kubernetes-sigs#5051)
  fixed golangci-lint config
  updated MD files
  updated MD files
  feat(chart): automate helm json schema (kubernetes-sigs#5075)
  docs(proposal): update proposal template, add statuses
  test(aws): introduce first fixture-based (kubernetes-sigs#5092)
  chore(makefile): add helper and document targets
  feat: Updated chart for v1.15.2 release
  chore(makefile): add helper and document targets
  chore(filter-tags): pre-process filter tags
  chore(filter-tags): pre-process filter tags
  chore(filter-tags): pre-process filter tags
  chore(deps): bump the dev-dependencies group across 1 directory with 21 updates
  update service.md, service.go
  ...
ivankatliarchuk added a commit to gofogo/k8s-sigs-external-dns-fork that referenced this pull request Feb 26, 2025
* master: (80 commits)
  chore(deps): bump the dev-dependencies group across 1 directory with 7 updates
  Update README.md with proper link to dev guide
  Add OpenStack Designate webook provider to readme
  chore(deps): bump the dev-dependencies group with 3 updates
  chore(deps): bump the dev-dependencies group with 20 updates
  chore(deps): bump azure/setup-helm in the dev-dependencies group
  style: formatting
  fix: remove broken test
  fix test name
  chore: upgrade ExternalDNS to go 1.24
  chore-makefile-coverage
  cover source.go getProviderSpecificAnnotations() with unit tests
  nitpick: rename cloudflare custom hostname test function
  review suggestions based improvements
  review suggestions
  fix(source): debug log on gateway target detection
  improve error message phrasing
  Update docs/sources/service.md
  chore(formatting): fix infected files with correct formatting (kubernetes-sigs#5099)
  docs: Fix managed-record-type argument
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants