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

Install systemd unit file if supported #19030

Closed

Conversation

dleske
Copy link

@dleske dleske commented Jul 26, 2023

Thank you for contributing to Harbor!

Comprehensive Summary of your change

On a systemd system, three or four of the necessary containers are started after a reboot of the 10 expected. Based on the advice in #18704, have added support for creating a systemd unit file in the install script. On a system without systemd, there is no effect.

Issue being fixed

Fixes #18704

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
    • Did not see guidelines on labels in the contributing guide. I don't believe this change requires adding anything to the release notes. This may be worth noting as an enhancement.
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
    • A cursory check through the tests directory indicates no tests make use of the install script.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.
    • I do not believe the documentation would benefit from any addition here.

@dleske dleske requested a review from a team as a code owner July 26, 2023 23:58
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #19030 (9960f13) into main (db82d6e) will decrease coverage by 22.60%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #19030       +/-   ##
===========================================
- Coverage   67.43%   44.83%   -22.60%     
===========================================
  Files         992      244      -748     
  Lines      108727    13386    -95341     
  Branches     2747     2747               
===========================================
- Hits        73316     6002    -67314     
+ Misses      31479     7080    -24399     
+ Partials     3932      304     -3628     
Flag Coverage Δ
unittests 44.83% <ø> (-22.60%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 754 files with indirect coverage changes

@OrlinVasilev OrlinVasilev added the release-note/update Update or Fix label Jul 27, 2023
@wy65701436
Copy link
Contributor

The installation script should not include the systemd service configuration. The purpose of the installation script is solely to handle the deployment of the Harbor service.

@wy65701436 wy65701436 closed this Jul 28, 2023
@dleske
Copy link
Author

dleske commented Jul 28, 2023

@wy65701436 I'm fine with the PR being rejected but the reasoning is unsound in my view. The install script's deployment is half-baked: it installs the service and starts it but does nothing to ensure it stays running. If I say I've deployed a service and it doesn't come back after a power outage or reboot, I haven't really deployed it.

Moreover, as the install script currently "deploys" the Harbor service, some of the containers are restarted on reboot, but fewer than half, leading to confusion and potentially a lack of trust in the software.

Finally, use of the unit file provided by Harbor is not documented as an installation step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/update Update or Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After reboot of Server harbor not working
4 participants