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

fix: fix Linux package scripts #800

Merged
merged 1 commit into from
Jan 21, 2025
Merged

fix: fix Linux package scripts #800

merged 1 commit into from
Jan 21, 2025
+46 −12

Conversation

netsandbox
Copy link
Contributor

fixes #799

@netsandbox netsandbox requested a review from a team as a code owner January 17, 2025 15:45
@netsandbox netsandbox requested a review from ChrsMark January 17, 2025 15:45
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jan 17, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: netsandbox / name: Christian Loos (ad35082)

@atoulme
Copy link
Contributor

atoulme commented Jan 17, 2025

Please include a changelog. Any way to test those changes?

@netsandbox
Copy link
Contributor Author

I have now added a changelog.

If the build rpm package works can be tested on a RHEL based OS host.

Actually the main fix from this MR, that the service is stopped and disabled after a rpm package update, can't be tested now, because when you install a package release with the fix from this MR, the package preuninstall script from the previous release will still stop and disable the service (see details on this in the linked issue #799).
You will only see if this works when you install a release, after the release with this MR fix.

@atoulme
Copy link
Contributor

atoulme commented Jan 20, 2025

Makes sense, that's an unhappy confluence of events, we should ideally add tests to test lifecycle after this release.

@netsandbox
Copy link
Contributor Author

I was also a little surprised that no one else ever noticed this since now.

It would make sense to create a release after this MR is merged, to get a fixed version out as soon as possible

@netsandbox
fix: fix Linux package scripts
ad35082 
* reload systemd daemon to make it aware of an updated service file
  after package update
* restart service to use the new binary after a package update
* don't stop and disable service after rpm package update

fixes open-telemetry#799
@netsandbox
Copy link
Contributor Author

I have just pushed changes again, to fix a copy&paste error I produced myself, and was detected by the build workflow.

mowies
mowies reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@mowies mowies left a comment •
edited
Loading

Choose a reason for hiding this comment

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

will your changes also work with the .deb package for debian based systems?
the shell scripts are also used for that package

@netsandbox
Copy link
Contributor Author

will your changes also work with the .deb package for debian based systems? the shell scripts are also used for that package

Yes, the changes work also for the Debian packages.
As the Debian packages use a different script order, they are not affected by this problem.

We just don't run the service stop and disable when $1 is 1, and this is only the case if the rpm package is upgraded. Debian package install never provides 1 for $1 to a prerm script, see https://wiki.debian.org/MaintainerScripts

@codeboten codeboten added this pull request to the merge queue Jan 21, 2025
Merged via the queue into open-telemetry:main with commit f109066 Jan 21, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MovieStoreGuy MovieStoreGuy MovieStoreGuy approved these changes

@mowies mowies mowies approved these changes

@codeboten codeboten codeboten approved these changes

@atoulme atoulme atoulme approved these changes

@ChrsMark ChrsMark Awaiting requested review from ChrsMark ChrsMark is a code owner automatically assigned from open-telemetry/collector-contrib-approvers

Assignees
No one assigned
Labels
release:blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Systemd service stopped and disabled after rpm package update
5 participants
@netsandbox @atoulme @codeboten @mowies @MovieStoreGuy