Skip to content

Install Scripts: add updater package#25479

Merged
marcoandredinis merged 1 commit intomasterfrom
marco/agent_updater_install_scripts
May 10, 2023
Merged

Install Scripts: add updater package#25479
marcoandredinis merged 1 commit intomasterfrom
marco/agent_updater_install_scripts

Conversation

@marcoandredinis
Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis commented May 2, 2023

This was manually tested and worked as expected.

Fixes #24628 (comment)

@marcoandredinis marcoandredinis force-pushed the marco/agent_updater_install_scripts branch 2 times, most recently from c10848a to 6a647fc Compare May 2, 2023 15:35
@marcoandredinis marcoandredinis force-pushed the marco/agent_updater_install_scripts branch from 6a647fc to 12afada Compare May 3, 2023 07:07
@marcoandredinis marcoandredinis marked this pull request as ready for review May 3, 2023 10:55
@github-actions github-actions Bot requested review from Tener and zmb3 May 3, 2023 10:55
@marcoandredinis marcoandredinis requested a review from lxea May 3, 2023 11:26
@r0mant r0mant requested a review from hugoShaka May 3, 2023 16:09

PACKAGE_LIST="{{ .TeleportPackage }}"
if [[ "{{ .AutomaticUpgrades }}" == "true" ]]; then
PACKAGE_LIST="${PACKAGE_LIST} {{ .TeleportPackage }}-updater"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't we need to install only xxx-updater package? I think it will pull teleport package as dependency?

@hugoShaka @fheinecke Am I wrong?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does 👍
https://apt.releases.development.teleport.dev/ubuntu/dists/bionic/stable/v13/binary-amd64/Packages
image

We can remove it, however I would like to clarify something first:
teleport-ent-updater depends on teleport
teleport and teleport-ent both provide teleport.

So, when we try to install only teleport-ent-updater which package is going to be pulled? Is it teleport or teleport-ent?

Copy link
Copy Markdown
Contributor

@hugoShaka hugoShaka May 3, 2023

Choose a reason for hiding this comment

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

Yes, but we don't want the teleport-ent package to be deleted if the user wants to uninstall the updater. IMO it makes sense to mark both as wanted, so teleport-ent won't be auto-removed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@hugoShaka In your documentation guide we instruct people to just apt install teleport-ent-updater. Is that for scenario when you already have an agent and want to add updater to it?

@fheinecke I think teleport-ent-updater should depend on teleport-ent, not teleport. Can we update that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is that for scenario when you already have an agent and want to add updater to it?

Yes, the guide explains how to enroll an existing agent into automatic updates

Copy link
Copy Markdown
Contributor

@fheinecke fheinecke May 3, 2023

Choose a reason for hiding this comment

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

I think teleport-ent-updater should depend on teleport-ent, not teleport. Can we update that?

@r0mant it has been (tested before I went on vacation, see https://github.com/gravitational/teleport.e/pull/1177)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we don't want the teleport-ent package to be deleted if the user wants to uninstall the updater.

I'll keep both packages then 👍

@r0mant r0mant requested review from fheinecke and removed request for Tener and lxea May 3, 2023 16:11
@zmb3 zmb3 removed their request for review May 3, 2023 16:58

PACKAGE_LIST="{{ .TeleportPackage }}"
if [[ "{{ .AutomaticUpgrades }}" == "true" ]]; then
PACKAGE_LIST="${PACKAGE_LIST} {{ .TeleportPackage }}-updater"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any condition where {{ .TeleportPackage }} is set to anything but teleport-ent-updater? We don't currently publish separate packages for OSS and this will error if it's set to teleport-updater.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It won't 👍

AutomaticUpgrades comes from:

automaticUpgrades := feats.AutomaticUpgrades && modules.GetModules().BuildType() == modules.BuildEnterprise

So, it will only be true for Enterprise builds
I left a comment so that we can comeback to this and simplify the condition only:

// TODO(marco): remove BuildType check when teleport-upgrade (oss) package is available in apt/yum repos

@marcoandredinis
Copy link
Copy Markdown
Contributor Author

@fheinecke Friendly ping

@marcoandredinis marcoandredinis added this pull request to the merge queue May 10, 2023
Merged via the queue into master with commit a2c4fd3 May 10, 2023
@marcoandredinis marcoandredinis deleted the marco/agent_updater_install_scripts branch May 10, 2023 07:34
@public-teleport-github-review-bot
Copy link
Copy Markdown

@marcoandredinis See the table below for backport results.

Branch Result
branch/v13 Create PR

marcoandredinis added a commit that referenced this pull request May 17, 2023
* Install Scripts: add updater package (#25479)

* Install Node Script: respect version variable (#25885)

We were always installing the latest version available even if the
requested version was different

This PR changes the installation command and the version is now pinned
to the one received in the `TELEPORT_VERSION` variable.

* revert teleport-upgrade installation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants