-
Notifications
You must be signed in to change notification settings - Fork 132
Updated directory structure and have versioned names #45
Conversation
binary/etcd.go
Outdated
return err | ||
} | ||
return util.CreateSymLink(etcdctlBinaryPath, etcdctlSymLinkPath) | ||
return util.CopyFile(etcdctlSrcPath, etcdctlDestPath) | ||
} | ||
|
||
// DeleteSymLinks deletes symlinks created for etcd binaires |
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.
nit: Change comment to reflect delete binaries
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.
Thanks Arun, fixed it
cmd/reset.go
Outdated
} | ||
// Remove symlinks | ||
if err = binary.DeleteSymLinks(constants.DefaultInstallBaseDir); err != nil { | ||
if err = binary.DeleteBinaries(constants.DefaultInstallBaseDir); err != nil { |
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.
If InstallBaseDir
is now renamed to InstallDir
, should DefaultInstallBaseDir
be renamed to DefaultInstallDir
?
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.
+1
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.
Thanks, updated it
binary/etcd.go
Outdated
|
||
if err := util.CreateSymLink(etcdBinaryPath, etcdSymLinkPath); err != nil { | ||
if err := util.CopyFile(etcdSrcPath, etcdDestPath); err != nil { |
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.
Since CopyFile
does not force copy a file cp -f
, should there be a FileExists
check to see if the binary already exists, and remove the binary if it does exist before copying the binary over to the destDir? (Idempotent operation)
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 thought that's already handled when we call IsInstalled
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.
Thanks updated to cp -f
binary/etcd.go
Outdated
etcdSymLinkPath := filepath.Join(symLinkDir, "etcd") | ||
etcdctlSymLinkPath := filepath.Join(symLinkDir, "etcdctl") | ||
if err := util.RemoveFile(etcdSymLinkPath); err != nil { | ||
func DeleteBinaries(installDir string) error { |
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.
Just FYI: I added an Uninstall
to complement InstallFromCache
in platform9@46866b8. Once that PR merges, this will get called by Uninstall
, and won't need to be exported.
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.
Thanks Daniel, updated to use Uninstall
util/filesystem.go
Outdated
@@ -42,3 +43,11 @@ func CreateSymLink(srcFile, linkFile string) error { | |||
RemoveFile(linkFile) | |||
return os.Symlink(srcFile, linkFile) | |||
} | |||
|
|||
// Copy file from src to dest | |||
func CopyFile(srcFile, destFile string) error { |
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.
It's true that the standard library doesn't provide a "copyfile" function (here's why). Do you prefer calling cp
to ioutil.ReadFile
+ ioutil.WriteFile
?
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.
yeah, i also tried to look at some external libraries e.g. https://github.com/spf13/afero but that also doesn't have a copy
utility. I felt that using cp
was much more concise and had other features like automatically retaining the file permissions..
util/filesystem.go
Outdated
@@ -42,3 +43,11 @@ func CreateSymLink(srcFile, linkFile string) error { | |||
RemoveFile(linkFile) | |||
return os.Symlink(srcFile, linkFile) | |||
} | |||
|
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.
FYI: The other functions (and this file) were removed in #46. You'll need to add the file back. My mistake. The file was not removed, but all functions apart from FileExists
were.
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.
sounds good, thanks Daniel
397795f
to
4628381
Compare
4628381
to
b2f56bf
Compare
Can you make sure this question is resolved https://github.com/platform9/isv-tests/pull/26#pullrequestreview-144584751? |
Updated
/var/cache
to have a consistent format.Updated directory names to have versions, to allow multiple installations to coexist
Have
/opt/bin
to have actual binaries and not symlinks (as we now have directories with versions in/var/cache
)Also change the directory names for other binaries to follow the same pattern of
/var/cache/<parent-tool>/<tool>/<tool-version>/<tool-binary>
(by parent tool we mean the one which uses the tool, e.g. ssh-provider is the parent tool for nodeadm, while nodeadm is the parent tool for kubernetes)contents of
/var/cache
contents of
/opt/bin