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

Adding gnupg to allow install on debian 9 and a warning if dirmngr is not installed #191

Merged
merged 1 commit into from
Aug 10, 2017

Conversation

hush-hush
Copy link
Member

@hush-hush hush-hush commented Aug 8, 2017

We don't enforce dependency to dirmngr on the .deb package since it's
shared between ubuntu and debian and it's only available in the
'universe' repo in ubuntu <= 14.04.

Packaging for debian and ubuntu will be split in future version of the
agent.

see: DataDog/dd-agent#3397

Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this 👍

Made a couple of comments.

Also, we can re-discuss in the future if it's worth separating the debian and ubuntu packages (given our current build system it might be a bit too complex for what we actually need). We only need apt-key to install the new APT key, when we switch to the new key (which could happen for the next major version of the agent for instance) we should be able to remove apt-key entirely from our install scripts

echo " ... OK"
else
echo " ... failed"
if [ -f "/etc/debian_version" ] || [ "$DISTRIBUTION" = "Debian" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

since /etc/debian_version is also present on Ubuntu, this condition will also be true on Ubuntu.

I think we can either:

  1. only use the $DISTRIBUTION value, which should work as long as lsb_release is actually present on the system (we can enforce that by declaring lsb-release as a dependency of the package), or
  2. change the error message a bit, to something like: If you're running Debian 9, [...]

Honestly I think option 2. is enough :)

else
echo " ... failed"
if [ -f "/etc/debian_version" ] || [ "$DISTRIBUTION" = "Debian" ]; then
echo "The dirmngr package is needed to add a new key to APT on Debian since version 9"
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding more details here on the consequences of failing to add the new APT key. Something like: WARNING: The "dirmngr" package is needed to install the new APT key required by future versions of the Agent. If this package is not installed, please install it and then re-install the agent. The agent installation may break in the future if the new APT key is not installed on your system

@hush-hush hush-hush force-pushed the maxime/debian-gnupg branch from 987fca7 to fd53991 Compare August 8, 2017 15:54
… not installed.

We don't enforce dependency to dirmngr on the .deb package since it's
shared between ubuntu and debian and it's only available in the
'universe' repo in ubuntu <= 14.04.
@hush-hush hush-hush force-pushed the maxime/debian-gnupg branch from fd53991 to bf72b4c Compare August 9, 2017 15:22
@hush-hush hush-hush merged commit 2b2dec4 into master Aug 10, 2017
@hush-hush hush-hush deleted the maxime/debian-gnupg branch August 10, 2017 08:03
@truthbk truthbk added this to the 5.18 milestone Aug 21, 2017
@olivielpeau olivielpeau modified the milestones: 5.17.0, 5.18 Aug 21, 2017
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.

3 participants