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

🌱 E2E: Add build tags #2042

Merged
merged 1 commit into from
Nov 8, 2024
Merged

🌱 E2E: Add build tags #2042

merged 1 commit into from
Nov 8, 2024

Conversation

lentzi90
Copy link
Member

@lentzi90 lentzi90 commented Nov 5, 2024 •
edited
Loading

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 ->

  • Current situation: ERROR ❌
  • With this PR: Success ✔️

@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 5, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented Nov 5, 2024

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
Copy link
Member Author

lentzi90 commented Nov 5, 2024

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

@metal3-io-bot
Copy link
Contributor

@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:

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kashifest
Copy link
Member

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
kashifest reviewed Nov 5, 2024
View reviewed changes
Makefile

.PHONY: build-vbmctl
build-vbmctl:
cd test; go build --tags=e2e,vbmctl -ldflags $(LDFLAGS) -o $(abspath $(BIN_DIR)/vbmctl) ./vbmctl/main.go
Copy link
Member

Choose a reason for hiding this comment

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

Do we also add build-vbmctl as part of build phony, as I see build-e2e is there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is better to leave it. If we add it htere, then libvirt-dev would be needed for make build. We will anyway build this code as part of the e2e pipeline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm now I wonder if we should do the same for golanci-lint also. It is not so nice to have them behave differently.

  • make lint -> lint everything
  • make build -> build everything except vbmctl

@kashifest
Copy link
Member

/retest

@lentzi90
Copy link
Member Author

lentzi90 commented Nov 5, 2024

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

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented Nov 5, 2024

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.

@lentzi90
E2E: Add build tags
ceb01bf 
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.

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

lentzi90 commented Nov 5, 2024

/retest

1 similar comment
@lentzi90
Copy link
Member Author

lentzi90 commented Nov 5, 2024

/retest

@lentzi90
Copy link
Member Author

lentzi90 commented Nov 6, 2024

These fixture test failures are driving me mad... Attempting to fix in #2043

@mquhuy
Copy link
Member

mquhuy commented Nov 6, 2024

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
Copy link
Member

mquhuy commented Nov 6, 2024

/retest

@mquhuy
Copy link
Member

mquhuy commented Nov 7, 2024

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented Nov 7, 2024

/hold cancel
Tests are passing 🎉

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

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2024
@kashifest
Copy link
Member

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

@metal3-io-bot
Copy link
Contributor

[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:

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented Nov 7, 2024

/test test

@lentzi90
Copy link
Member Author

lentzi90 commented Nov 8, 2024

/test test

@tuminoid
Copy link
Member

tuminoid commented Nov 8, 2024

/retest

@lentzi90
Copy link
Member Author

lentzi90 commented Nov 8, 2024

We seem to be hitting the golanci-lint deadline. Fixing here: #2046

@metal3-io-bot metal3-io-bot merged commit 70bc8d4 into metal3-io:main Nov 8, 2024
24 of 27 checks passed
@metal3-io-bot metal3-io-bot deleted the lentzi90/e2e-build-tag branch November 8, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kashifest kashifest kashifest left review comments

@dtantsur dtantsur Awaiting requested review from dtantsur

@zaneb zaneb Awaiting requested review from zaneb

@mquhuy mquhuy Awaiting requested review from mquhuy

Assignees
No one assigned
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lentzi90 @metal3-io-bot @kashifest @mquhuy @tuminoid