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

Run all release builds on PRs #85

Closed
mx-psi opened this issue Mar 17, 2022 · 9 comments · Fixed by #91 or #94
Closed

Run all release builds on PRs #85

mx-psi opened this issue Mar 17, 2022 · 9 comments · Fixed by #91 or #94
Assignees
Labels
good first issue Good for newcomers

Comments

@mx-psi
Copy link
Member

mx-psi commented Mar 17, 2022

Currently, CI for PRs on this repository does not build all the artifacts. This makes it hard to catch issues before actually tagging a release. To prevent this we can do a gorelease dry run on all PRs.

@mx-psi mx-psi added the good first issue Good for newcomers label Mar 17, 2022
@mx-psi
Copy link
Member Author

mx-psi commented Mar 17, 2022

This would need to be modified here

args: --snapshot --rm-dist --timeout 1h

@jpkrohling
Copy link
Member

I think it might be better to have a nightly build, trying the current tip of the main branches from contrib and core. Running go-releaser would still make sense for PRs touching the go-releaser config.

Was the 386 problem caused by a new component, or by a change in an existing component?

@mx-psi
Copy link
Member Author

mx-psi commented Mar 17, 2022

I think it might be better to have a nightly build, trying the current tip of the main branches from contrib and core. Running go-releaser would still make sense for PRs touching the go-releaser config.

That also sounds like a good idea

Was the 386 problem caused by a new component, or by a change in an existing component?

It was caused by a bump on the github.com/open-telemetry/opentelemetry-log-collection dependency, which is used by many (all?) log receivers

@jpkrohling
Copy link
Member

It was caused by a bump on the github.com/open-telemetry/opentelemetry-log-collection dependency, which is used by many (all?) log receivers

So, it means that unless we had a change on this repo, we wouldn't have caught this in time. A periodic/daily build would probably have caught this, except if the bug was introduced less than 24h before the release.

@Aneurysm9
Copy link
Member

I think building on PR here is the most appropriate thing for this repo. It should give immediate feedback on changes that could impact the release of artifacts from this repo. The source repos should be responsible for ensuring that their head and release tags build appropriately as well, which I think makes the utility of a nightly build here limited unless we intend to also publish those artifacts for users who want to live on the bleeding edge.

@jpkrohling
Copy link
Member

Perhaps I'm missing something, but I'm not sure running the goreleaser step on every PR will help.

It takes a long time and running it on every PR shouldn't bring any benefits if the PR isn't touching the goreleaser file, given that the manifests are using static tags (v0.46.0). Unless the tags are changing, the output will always be the same given the same goreleaser config.

@mx-psi
Copy link
Member Author

mx-psi commented Mar 17, 2022

My point (and I think @Aneurysm9's probably is too) is that if we run it on PRs we would have caught it before merging #81, and we would have avoided the tag-then-untag hack. I don't know if running in all PRs is the right answer, but at least on release-related PRs it should definitely do a full dry run.

@Aneurysm9
Copy link
Member

Yes, perhaps we don't need to run the full process on every PR, but certainly any PR that changes the builder manifests or goreleaser config.

@jpkrohling
Copy link
Member

any PR that changes the builder manifests or goreleaser config

We have an agreement here. Indeed, we should run the builds when the manifests have changed as well.

@jpkrohling jpkrohling self-assigned this Mar 21, 2022
jpkrohling added a commit to jpkrohling/opentelemetry-collector-releases that referenced this issue Mar 21, 2022
jpkrohling added a commit to jpkrohling/opentelemetry-collector-releases that referenced this issue Mar 21, 2022
Fixes open-telemetry#85

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling added a commit that referenced this issue Mar 22, 2022
Fixes #85

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling reopened this Mar 22, 2022
jpkrohling added a commit to jpkrohling/opentelemetry-collector-releases that referenced this issue Mar 22, 2022
This is the second attempt to get this fixed.

Fixes open-telemetry#85

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling added a commit that referenced this issue Mar 23, 2022
* Run full build when manifests change

This is the second attempt to get this fixed.

Fixes #85

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

* Revert bogus change

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
3 participants