-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
lib/update/system/system.go
Outdated
} | ||
|
||
// renderTctlScript renders the contents of the script that invokes tctl binary | ||
// with apporpriate configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apporpriate -> appropriate
logrus.ErrorKey: err, | ||
"output": string(out), | ||
}).Warn("Failed to update kubectl symlink.") | ||
logger.WithError(err).WithField("output", string(out)).Warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I want to add another field - should I chain it with another WithFeild
? Each WithField
is also potentially allocating memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, you can still use WithFields()
, I just think WithField()
looks cleaner when you have only 1 field.
// tctlScript is the template of the script that invokes tctl binary with | ||
// configuration supplied via an environment variable. | ||
var tctlScript = template.Must(template.New("tctl").Parse(`#!/bin/bash | ||
TELEPORT_CONFIG={{.conf}} {{.path}} "$@"`)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will path
be properly escaped if it contains special characters? Ditto for teleport configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think path can contain special characters. It points to the unpacked teleport package directory inside our state dir, and if somebody overrides state dir to have special characters in it, I think they'll have bigger problems than non-working tctl script.
And the configuration is base64-encoded so it doesn't need to be escaped.
tool/gravity/cli/system.go
Outdated
} | ||
|
||
args := []string{"system", "reinstall", newPackage.String()} | ||
args := []string{"system", "reinstall", newPackage.String(), "--cluster-role", clusterRole} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to flag description it's optional.
This PR makes sure that
tctl
can be used inside gravity clusters and is pre-configured so it can be used without any configuration from user's side. Refs https://github.com/gravitational/gravity.e/issues/4198, #580.Also a couple of other things:
I'm actually thinking we should just nuke the whole "cli/system" module as 90% of its commands are not used anymore and moreover they're not kept up-to-date with the logic in "update/system" so it's probably even harmful to use them. Refs https://github.com/gravitational/gravity.e/issues/4175.
Fix the issue with helm script symlink not being updated during upgrade (which was also a result of discrepancy between cli/system and update/system). Closes https://github.com/gravitational/gravity.e/issues/4173.
Update system uninstall to cleanup kubectl, helm and tctl links/scripts as well.