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

[.github] Simplify workflow #29286

Closed
wants to merge 3 commits into from

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Nov 15, 2023

Description:

I think we can get rid of these jobs, since the debs and rpms are created in opentelemetry-collector-releases.

@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 15, 2023
@mx-psi mx-psi marked this pull request as ready for review November 20, 2023 14:09
@mx-psi mx-psi requested a review from a team as a code owner November 20, 2023 14:09
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

I can't recall these checks ever revealing anything actionable, so fully agree with them being removed.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

The installation of the deb/rpm packages has definitely caught issues in the past where the service would fail to start. I'd recommend adding a similar test somewhere if we don't want the checks here. Pinging @atoulme who reminded me of this on another issue where i suggested the same thing

@mx-psi
Copy link
Member Author

mx-psi commented Nov 29, 2023

The installation of the deb/rpm packages has definitely caught issues in the past where the service would fail to start

Shouldn't this be covered by e2e tests? It doesn't make a lot of sense to me to build a deb/rpm that we never end up using just for this.

@codeboten
Copy link
Contributor

Shouldn't this be covered by e2e tests? It doesn't make a lot of sense to me to build a deb/rpm that we never end up using just for this.

Sure, I don't know if it's possible to run the installation test without an installable package :) This might be a chicken and egg problem. I'm in support of adding these tests where it makes sense, I just don't want to lose the ability to test for regressions

@mx-psi
Copy link
Member Author

mx-psi commented Nov 29, 2023

Shouldn't this be covered by e2e tests? It doesn't make a lot of sense to me to build a deb/rpm that we never end up using just for this.

Sure, I don't know if it's possible to run the installation test without an installable package :) This might be a chicken and egg problem. I'm in support of adding these tests where it makes sense, I just don't want to lose the ability to test for regressions

Ahhh, so you are talking about the systemd service, not the Collector service (as in, go.opentelemetry.io/collector/service). Sorry, I got confused there by the term 😄

I guess then my question is... is the systemd service on this repository consistent with the systemd service on releases? Do we have any automated way of making sure they keep on being consistent over time?

I am tempted to close this and file an issue on releases to add these tests there, since that seems like the right place to test things in (with the final artifacts that we actually release to users). Would that make sense to you?

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@mx-psi
Copy link
Member Author

mx-psi commented Dec 14, 2023

@mx-psi mx-psi closed this Dec 14, 2023
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 Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants