search

metal3-io / baremetal-operator

Bare metal host provisioning integration for Kubernetes
Apache License 2.0
592 stars 253 forks source link

🌱 E2E: Add build tags #2042

Closed lentzi90 closed 2 weeks ago

lentzi90 commented 3 weeks ago

This makes use of build tags for the e2e tests and related tools. The goal is to make it easier to compile and develop BMO by relaxing the requirement for libvirt bindings. They are now only needed when working on the relevant code.

One exception is golangci-lint. In order for it to go through all code, we need to include the build tags when running the linters. Developers that want to run golangci-lint will need the libvirt bindings.

What this PR does / why we need it:

Developer doesn't have libvirt-dev installed and runs make unit ->

lentzi90 commented 3 weeks ago

Maybe also worth mentioning that at least CAPO and CAPA also uses the e2e build tag in this way to keep the e2e code separated from the rest.

lentzi90 commented 3 weeks ago

/cc @mquhuy @kashifest I would value your input on this

metal3-io-bot commented 3 weeks ago

@lentzi90: GitHub didn't allow me to request PR reviews from the following users: on, would, your, input, I, value, this.

Note that only metal3-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/metal3-io/baremetal-operator/pull/2042#issuecomment-2456908175): >/cc @mquhuy @kashifest I would value your input on this Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
kashifest commented 3 weeks ago

This would be useful. Thanks for adding it.

One exception is golangci-lint. In order for it to go through all code, we need to include the build tags when running the linters. Developers that want to run golangci-lint will need the libvirt bindings.

Do we need to change something for this? I can see we only run golangci-lint locally, we dont build/tag anything?

kashifest commented 3 weeks ago

/retest

lentzi90 commented 3 weeks ago

/hold I see I have missed to change the path in some place. Let me fix that.

lentzi90 commented 3 weeks ago

Do we need to change something for this? I can see we only run golangci-lint locally, we dont build/tag anything?

If we want golangci-lint to check the code that binds to libvirt there is no way around it unfortunately. I'm not sure how golangci-lint works, but in some sense it must be building the code because it sees the same error as go build does if the libvirt-dev package isn't installed.

We could chose to split the linting so that test/vbmctl would be a separate target. Then it would be possible to run make lint without libvirt bindings. However, then there is the risk that we forget to run the separate target. I don't have a strong opinion on what is better.

We could for example have it like this:

lentzi90 commented 3 weeks ago

/retest

lentzi90 commented 3 weeks ago

/retest

lentzi90 commented 2 weeks ago

These fixture test failures are driving me mad... Attempting to fix in https://github.com/metal3-io/baremetal-operator/pull/2043

mquhuy commented 2 weeks ago

Do we need to change something for this? I can see we only run golangci-lint locally, we dont build/tag anything?

If we want golangci-lint to check the code that binds to libvirt there is no way around it unfortunately. I'm not sure how golangci-lint works, but in some sense it must be building the code because it sees the same error as go build does if the libvirt-dev package isn't installed.

We could chose to split the linting so that test/vbmctl would be a separate target. Then it would be possible to run make lint without libvirt bindings. However, then there is the risk that we forget to run the separate target. I don't have a strong opinion on what is better.

We could for example have it like this:

  • make lint: Only checks code without build tags. Works without libvirt-dev.
  • make lint-e2e: Only checks code under test. Requires libvirt-dev.

No strong preference from me, but imo it would be good to not have to install libvirt-dev in the lint job.

We could repurpose the current lint workflow to be the lint-e2e, and config it so that it only runs when we edit the code inside the test/vbmctl directory.

Otherwise, I think this PR is a nice improvement. Not sure if any developer will ever work on this repo without installing libvirt-dev, but it's nice to at least have options.

mquhuy commented 2 weeks ago

/retest

mquhuy commented 2 weeks ago

/lgtm

lentzi90 commented 2 weeks ago

/hold cancel Tests are passing :tada:

I'm still open to discuss how we handle the linters, but if you are happy like this, then lets merge @kashifest

kashifest commented 2 weeks ago

Lets merge it and see if the we need a change in the linter once we hit an issue /approve

metal3-io-bot commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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: - ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)~~ [kashifest] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
lentzi90 commented 2 weeks ago

/test test

lentzi90 commented 2 weeks ago

/test test

tuminoid commented 2 weeks ago

/retest

lentzi90 commented 2 weeks ago

We seem to be hitting the golanci-lint deadline. Fixing here: https://github.com/metal3-io/baremetal-operator/pull/2046

lentzi90 commented 1 day ago

/cherry-pick release-0.8

metal3-io-bot commented 1 day ago

@lentzi90: #2042 failed to apply on top of branch "release-0.8":

Applying: E2E: Add build tags
Using index info to reconstruct a base tree...
M   .golangci.yaml
M   Makefile
M   hack/ci-e2e.sh
M   test/e2e/basic_ops_test.go
M   test/e2e/cert_manager.go
M   test/e2e/common.go
M   test/e2e/e2e_config.go
M   test/e2e/e2e_suite_test.go
M   test/e2e/external_inspection_test.go
M   test/e2e/inspection_test.go
M   test/e2e/live_iso_test.go
M   test/e2e/provisioning_and_annotation_test.go
M   test/e2e/re_inspection_test.go
M   test/e2e/upgrade_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/upgrade_test.go
Auto-merging test/e2e/re_inspection_test.go
Auto-merging test/e2e/provisioning_and_annotation_test.go
Auto-merging test/e2e/live_iso_test.go
Auto-merging test/e2e/inspection_test.go
Auto-merging test/e2e/external_inspection_test.go
Auto-merging test/e2e/e2e_suite_test.go
Auto-merging test/e2e/e2e_config.go
Auto-merging test/e2e/common.go
Auto-merging test/e2e/cert_manager.go
Auto-merging test/e2e/basic_ops_test.go
Auto-merging hack/ci-e2e.sh
CONFLICT (content): Merge conflict in hack/ci-e2e.sh
Auto-merging Makefile
Auto-merging .golangci.yaml
CONFLICT (content): Merge conflict in .golangci.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 E2E: Add build tags
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
In response to [this](https://github.com/metal3-io/baremetal-operator/pull/2042#issuecomment-2497128314): >/cherry-pick release-0.8 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.