Skip to content

Introduce a sanity workflow for checking static linting violations in CI #2346

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

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

timflannagan
Copy link
Member

Update the Makefile and introduce a lint target. This target is responsible for running both gofmt and goimports against non-vendor packages in the codebase.

Introduce the ci/sanity GH workflow:

  • Runs make vendor
  • Runs make lint
  • Ensures that both of those targets don't produce a diff when running
    make diff

Fix any linting gofmt/goimports linting violations that came up when running the make lint target locally.

Makefile Outdated
Comment on lines 188 to 189
find . -name '*.go' -not -path "./vendor/*" | xargs gofmt -w
find . -name '*.go' -not -path "./vendor/*" | xargs goimports -w
Copy link
Member Author

Choose a reason for hiding this comment

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

This likely isn't the greatest structure but I didn't have an immediate answer on how to clean it up.

It looks like we do some combination of the following:

  • Not run both of those checks
  • Make the list of files spit out by that find command a prerequisite and update the xargs command to run both gofmt/goimports references that same set of files.

Explicitly ignore operatorclient generated packages

The gofmt/goimport commands are touching generated packages that don't
have the proper "//go:generate ..." instructions.

Signed-off-by: timflannagan <[email protected]>
…style checks

Introduce the ci/sanity GH workflow:
- Runs `make vendor`
- Runs `make lint`
- Ensures that both of those targets don't produce a diff when running
  `make diff`

Signed-off-by: timflannagan <[email protected]>
Run find . -name '*.go' -not -path "./vendor/*" -not -path "./pkg/lib/operatorclient/operatorclientmocks/*" | xargs gofmt -w
find . -name '*.go' -not -path "./vendor/*" -not -path "./pkg/lib/operatorclient/operatorclientmocks/*" | xargs goimports -w and commit the results.

Signed-off-by: timflannagan <[email protected]>
@timflannagan
Copy link
Member Author

#2091

@@ -184,6 +184,9 @@ clean:
@rm -rf test/e2e/log
@rm -rf e2e.namespace

lint:
find . -name '*.go' -not -path "./vendor/*" -not -path "./pkg/lib/operatorclient/operatorclientmocks/*" | xargs gofmt -w
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: mentioned this in the commit, but that operatorclientmocks package doesn't have the //go:generate ... comment that it looks like gofmt/goimports are configured to ignore, so we'd need to explicitly ignore this path from the find command output.

@njhale
Copy link
Member

njhale commented Sep 10, 2021

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2021
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2021
@openshift-ci
Copy link

openshift-ci bot commented Sep 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, njhale, timflannagan

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

@timflannagan
Copy link
Member Author

Manually merging - that unit test is permafailing (#2091).

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@timflannagan timflannagan merged commit 851c5e4 into operator-framework:master Sep 10, 2021
@timflannagan timflannagan deleted the add-sanity-check branch September 10, 2021 17:04
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants