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

debian package: improve wording in postinst script #3109

Closed
wants to merge 3 commits into from

Conversation

mdartmann
Copy link

Description

This pull request changes the wording in output of the postinst script of the new Debian packages for OpenSearch and OpenSearch Dashboards.

  • Remove the call for a daemon-reload. This is already done in the postinst script and thus running it again would be redundant.
  • Remove sudo from the commands for the following reasons:
    • dpkg already has to be run as root to install the package
    • sudo is not shipped by Debian in all cases and other tools exist (such as doas, su, etc.)
    • systemctl can use Polkit to ask for a password when a user session exists which would make sudo redundant
  • Change the wording and fix some grammatical issues

Let me know if you'd like me to revise this or if you have any other input. Thanks!

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Mae Dartmann added 3 commits January 17, 2023 10:03
since a daemon reload already occurs in the postinst script, it is
redundant to prompt the user to run it again.

Signed-off-by: Mae Dartmann <[email protected]>

 Changes to be committed:
	modified:   scripts/pkg/build_templates/opensearch-dashboards/deb/debian/postinst
	modified:   scripts/pkg/build_templates/opensearch/deb/debian/postinst
Adding `sudo` here is redundant, because:
- `dpkg` already needs to be running as root to install the package
- `sudo` is not always preinstalled on Debian and other tools exist
- `systemctl` can use `polkit` to prompt for a password by itself

Signed-off-by: Mae Dartmann <[email protected]>

 Changes to be committed:
	modified:   scripts/pkg/build_templates/opensearch-dashboards/deb/debian/postinst
	modified:   scripts/pkg/build_templates/opensearch/deb/debian/postinst
add missing `the` in front of name, remove redundant `using systemd`
when already using `systemctl`

Signed-off-by: Mae Dartmann <[email protected]>

 Changes to be committed:
	modified:   scripts/pkg/build_templates/opensearch-dashboards/deb/debian/postinst
	modified:   scripts/pkg/build_templates/opensearch/deb/debian/postinst
@mdartmann mdartmann requested a review from a team as a code owner January 17, 2023 09:59
@mdartmann
Copy link
Author

This is related to #28

@prudhvigodithi
Copy link
Collaborator

Hey @mdartmann thanks for PR LGTM, @peterzhuamazon can you please add your thoughts?
Thank you

@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2023

Codecov Report

Merging #3109 (67389a5) into main (d495899) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3109   +/-   ##
=======================================
  Coverage   93.17%   93.17%           
=======================================
  Files         167      167           
  Lines        4602     4602           
=======================================
  Hits         4288     4288           
  Misses        314      314           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Hi @mdartmann,

Thanks for the contribution.

I would block this a bit as we are currently in the process of 2.5.0 release.
And we are using the same description in RPM to DEB.

I do not see any issue to keep the systemctl daemon-reload text as we want to be clear on the steps to users.

Adding or removing sudo might be a bigger debate and I fully understand your comment on this.

If we have to change this we need to change RPM as well for similar reason.

I would involve more into this before we get it merged.

cc: @bbarani @dblock @setiah

Thanks.

@bbarani
Copy link
Member

bbarani commented Mar 15, 2023

@peterzhuamazon Can we merge this PR now? Please add your comments.

Copy link
Contributor

@smortex smortex left a comment

Choose a reason for hiding this comment

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

+1 for better description and removal of unneeded steps

@peterzhuamazon
Copy link
Member

We will close this PR for now as there is currently no plan to change the description.
Maybe we can revisit this once we move to the next major version.

Thanks!.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants