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.

Sign up for GitHub

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

Jump to bottom

project-infra: add make targets lint and coverage #3825

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

project-infra: add make targets lint and coverage #3825

wants to merge 6 commits into from
+119 −5

Conversation

dhiller
Copy link
Contributor

@dhiller dhiller commented Dec 12, 2024 •
edited
Loading

What this PR does / why we need it:

Adds the coverage and lint make targets and the initial .golangci-lint configuration.

Also it adds //nolint directive to some of the files.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3822

Special notes for your reviewer:

@kubevirt-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 12, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from dhiller. For more information see the Kubernetes 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:

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

@dhiller
lint: add make target lint
7b47022 
Adds the lint make target and the initial .golangci-lint configuration.

Also it adds //nolint directive to some of the files

Signed-off-by: Daniel Hiller <[email protected]>
@dhiller
coverage: add make target
5190e56 
Adds a make target that writes a coverage report to ARTIFACTS output
directory sub directory.

Signed-off-by: Daniel Hiller <[email protected]>
@dhiller
project-infra: add presubmits for lint and coverage
1681f2b 
Signed-off-by: Daniel Hiller <[email protected]>
@dhiller
Copy link
Contributor Author

dhiller commented Dec 16, 2024

/rehearse ?

@kubevirt-bot
Copy link
Contributor

Rehearsal is available for the following jobs in this PR:

pull-project-infra-lint
pull-project-infra-coverage

You can trigger rehearsal for all jobs by commenting either /rehearse or /rehearse all
on this PR.

For a specific PR you can comment /rehearse {job-name}.

For a list of jobs that you can rehearse you can comment /rehearse ?.

@dhiller
Copy link
Contributor Author

dhiller commented Dec 16, 2024

/rehearse

@kubevirt-bot
Copy link
Contributor

Rehearsal jobs created for this PR:

rehearsal-pull-project-infra-lint
rehearsal-pull-project-infra-coverage

You can trigger rehearsal for all jobs by commenting either /rehearse or /rehearse all
on this PR.

For a specific PR you can comment /rehearse {job-name}.

For a list of jobs that you can rehearse you can comment /rehearse ?.

@dhiller dhiller marked this pull request as ready for review December 16, 2024 12:47
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2024
@kubevirt-bot kubevirt-bot requested a review from xpivarc December 16, 2024 12:47
@dhiller dhiller changed the title lint: add make target lint project-infra: add make targets lint and coverage Dec 16, 2024
@dhiller dhiller marked this pull request as draft December 16, 2024 16:49
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2024
@dhiller
add ginkgo
22defd1 
Signed-off-by: Daniel Hiller <[email protected]>
@dhiller
Copy link
Contributor Author

dhiller commented Dec 17, 2024

/rehearse pull-project-infra-coverage

@kubevirt-bot
Copy link
Contributor

Rehearsal jobs created for this PR:

rehearsal-pull-project-infra-coverage

You can trigger rehearsal for all jobs by commenting either /rehearse or /rehearse all
on this PR.

For a specific PR you can comment /rehearse {job-name}.

For a list of jobs that you can rehearse you can comment /rehearse ?.

@dhiller
Copy link
Contributor Author

dhiller commented Dec 17, 2024

/rehearse pull-project-infra-lint

@kubevirt-bot
Copy link
Contributor

Rehearsal jobs created for this PR:

rehearsal-pull-project-infra-lint

You can trigger rehearsal for all jobs by commenting either /rehearse or /rehearse all
on this PR.

For a specific PR you can comment /rehearse {job-name}.

For a list of jobs that you can rehearse you can comment /rehearse ?.

@dhiller
Copy link
Contributor Author

dhiller commented Dec 17, 2024

Coverage report here: https://storage.googleapis.com/kubevirt-prow/pr-logs/pull/kubevirt_project-infra/3825/rehearsal-pull-project-infra-coverage/1868944261240590336/artifacts/coverage.html

@dhiller
Copy link
Contributor Author

dhiller commented Dec 17, 2024

/rehearse pull-project-infra-lint

@kubevirt-bot
Copy link
Contributor

Rehearsal jobs created for this PR:

rehearsal-pull-project-infra-lint

You can trigger rehearsal for all jobs by commenting either /rehearse or /rehearse all
on this PR.

For a specific PR you can comment /rehearse {job-name}.

For a list of jobs that you can rehearse you can comment /rehearse ?.

@dhiller dhiller marked this pull request as ready for review December 17, 2024 11:44
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 17, 2024
@dhiller
Copy link
Contributor Author

dhiller commented Dec 17, 2024

@brianmcarey mind taking a look? Rehearsal jobs work as expected. (Have decided to add the status in another PR.)

brianmcarey
brianmcarey reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

thanks @dhiller for this - added a couple of comments inline.

Makefile Outdated
all: deps-update $(limiter) $(flake-report-writer) $(querier) $(kubevirtci) $(flake-issue-creator)

clean:
make-artifacts-dir:
Copy link
Member

Choose a reason for hiding this comment

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

As this is only used by the coverage target - can it be included there? rather than having its own target?

.golangci.yaml Show resolved Hide resolved
releng/release-tool/release-tool.go
@@ -772,7 +772,7 @@ func (r *releaseData) getReleases() ([]*github.RepositoryRelease, error) {
return r.allReleases, nil
}

func (r *releaseData) autoDetectData(autoReleaseCadance string, autoPromoteAfterDays int) error {
func (r *releaseData) autoDetectData(autoReleaseCadance string, autoPromoteAfterDays int) error { //nolint:gocyclo
Copy link
Member

Choose a reason for hiding this comment

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

Are these nolints here just to get something running initially?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, also the git grep is to remind us to remove them

github/ci/prow-deploy/files/jobs/kubevirt/project-infra/project-infra-presubmits.yaml
containers:
- args:
- |
GOBIN=/usr/local/bin/ go install github.com/onsi/ginkgo/v2/[email protected]
Copy link
Member

Choose a reason for hiding this comment

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

Having a specific image for this would remove the need for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true - but since we only need this in two cases currently I am not sure whether it's worth it. Also I don't want to put this into the golang image, doesn't seem like a good fit. Maybe some project-infra-test image? WDYT?
(I am not really sure whether we would want to tackle that in this PR, maybe create an issue as tracker?)

@dhiller dhiller mentioned this pull request Dec 17, 2024
Open
8 tasks
@dhiller
Copy link
Contributor Author

dhiller commented Dec 17, 2024

@brianmcarey addressed comments, PTAL thanks!

@dhiller dhiller requested a review from brianmcarey December 17, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enp0s3 enp0s3 Awaiting requested review from enp0s3

@xpivarc xpivarc Awaiting requested review from xpivarc

@brianmcarey brianmcarey Awaiting requested review from brianmcarey

Assignees
No one assigned
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lint: integrate golangci-lint into project setup
3 participants
@dhiller @kubevirt-bot @brianmcarey