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.

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

Clean up local workflow #2774

Merged

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Mar 21, 2024

Clean up and reorganize make targets used most frequently during local development. The intent is to make the new contributor experience better and to avoid unnecessary dependencies between the targets.

The actual changes are as follows:

  • make test no longer runs anything other than tests. Previously it also ran fmt, vet and generate.
  • make manager no longer does anything other than building the manager binary. Previously it depended on some other unnecessary targets.
  • make precommit is a new target that includes all the local checks, excluding e2e tests.
  • Updated CONTRIBUTING.md to reflect the changes and removed some outdated info.

@swiatekm swiatekm added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Mar 21, 2024
@swiatekm swiatekm requested a review from a team March 21, 2024 14:02
@swiatekm swiatekm force-pushed the chore/local-makefile-targets branch from fd1d79b to 27a8375 Compare March 26, 2024 16:32
@swiatekm swiatekm requested a review from jaronoff97 March 26, 2024 16:32
Makefile Outdated

# Build manager binary
.PHONY: manager
manager: generate fmt vet
manager:
Copy link
Member

Choose a reason for hiding this comment

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

generate generates deepcopy methods for the api package wich seems fundamental for manager binary

Copy link
Contributor Author

@swiatekm swiatekm Mar 29, 2024

Choose a reason for hiding this comment

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

I don't think there's any reason to run it every time we build the manager, though. We don't do this for other binaries.

I'm generally of the opinion that targets like generate shouldn't be too implicit. I'm not sure whether including it as a dependency everywhere isn't more confusing to a new contributor vs having to run it manually when they make changes to api types.

Copy link
Member

Choose a reason for hiding this comment

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

The majority of contributors don't know what the generate command does. There is not even documentation that for the change on the objects in the api package the command should be run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually documented here: https://github.com/open-telemetry/opentelemetry-operator/blob/main/CONTRIBUTING.md#make-changes-to-the-project-manifests.

But in general, I think this is ok. If the contributor should've run generate, but didn't, the CI will catch that, as will running make precommit.

What I'm trying to achieve with this PR is to not surprise users with changes to files in version control. Running tests or building the operator binary should not do this, in my view, even if it may be convenient at times.

Copy link
Member

Choose a reason for hiding this comment

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

I will not argue anymore but it mentions manifests not the API package.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine splitting manifest generation from the compilation. But I would expect that compilation produces up-to-date binaries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see that argument. In that case, should I also add generate to the opamp bridge make target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record, I definitely disagree with running fmt as part of this target, so I guess me and operator-sdk are enemies now. 🗡️

Copy link
Member

Choose a reason for hiding this comment

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

I think we should be consistent and if opapm requires the generate command it should be added as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added generate as a dependency of both manager and opamp-bridge.

@swiatekm swiatekm force-pushed the chore/local-makefile-targets branch from 27a8375 to c44bfb5 Compare April 3, 2024 10:45
@swiatekm swiatekm requested a review from pavolloffay April 3, 2024 10:46
@pavolloffay pavolloffay merged commit 95586ff into open-telemetry:main Apr 5, 2024
31 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* bump to latest opamp (open-telemetry#2807)

* Release 0.97.0 (open-telemetry#2806)

* Release 0.97.0

* oops
''

* Quiets some SUPER noisy logs (open-telemetry#2812)

* Clean up local workflow

---------

Co-authored-by: Jacob Aronoff <[email protected]>
@swiatekm swiatekm deleted the chore/local-makefile-targets branch November 30, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants