From 9360887551ad8ffac5b45589e217d2794ab50502 Mon Sep 17 00:00:00 2001 From: Justin SB Date: Wed, 30 Jan 2019 14:13:47 -0500 Subject: [PATCH] Refactor names of URLs in assets to clarify their purpose --- pkg/assets/builder.go | 55 +++++++++++++++++------------------ upup/pkg/fi/cloudup/loader.go | 14 ++++----- upup/pkg/fi/dryrun_target.go | 8 ++--- 3 files changed, 37 insertions(+), 40 deletions(-) diff --git a/pkg/assets/builder.go b/pkg/assets/builder.go index 2369a8ae53..9a43ca1e3b 100644 --- a/pkg/assets/builder.go +++ b/pkg/assets/builder.go @@ -64,10 +64,10 @@ type ContainerAsset struct { // FileAsset models a file's location. type FileAsset struct { - // FileURL is the URL of a file that is accessed by a Kubernetes cluster. - FileURL *url.URL - // CanonicalFileURL is the source URL of a file. This is used to copy a file to a FileRepository. - CanonicalFileURL *url.URL + // DownloadURL is the URL from which the cluster should download the asset. + DownloadURL *url.URL + // CanonicalURL is the canonical location of the asset, for example as distributed by the kops project + CanonicalURL *url.URL // SHAValue is the SHA hash of the FileAsset. SHAValue string } @@ -208,18 +208,18 @@ func (a *AssetBuilder) RemapFileAndSHA(fileURL *url.URL) (*url.URL, *hashing.Has } fileAsset := &FileAsset{ - FileURL: fileURL, + DownloadURL: fileURL, } if a.AssetsLocation != nil && a.AssetsLocation.FileRepository != nil { - fileAsset.CanonicalFileURL = fileURL + fileAsset.CanonicalURL = fileURL - normalizedFileURL, err := a.normalizeURL(fileURL) + normalizedFileURL, err := a.remapURL(fileURL) if err != nil { return nil, nil, err } - fileAsset.FileURL = normalizedFileURL + fileAsset.DownloadURL = normalizedFileURL glog.V(4).Infof("adding remapped file: %+v", fileAsset) } @@ -233,7 +233,7 @@ func (a *AssetBuilder) RemapFileAndSHA(fileURL *url.URL) (*url.URL, *hashing.Has a.FileAssets = append(a.FileAssets, fileAsset) glog.V(8).Infof("adding file: %+v", fileAsset) - return fileAsset.FileURL, h, nil + return fileAsset.DownloadURL, h, nil } // TODO - remove this method as CNI does now have a SHA file @@ -245,25 +245,25 @@ func (a *AssetBuilder) RemapFileAndSHAValue(fileURL *url.URL, shaValue string) ( } fileAsset := &FileAsset{ - FileURL: fileURL, - SHAValue: shaValue, + DownloadURL: fileURL, + SHAValue: shaValue, } if a.AssetsLocation != nil && a.AssetsLocation.FileRepository != nil { - fileAsset.CanonicalFileURL = fileURL + fileAsset.CanonicalURL = fileURL - normalizedFile, err := a.normalizeURL(fileURL) + normalizedFile, err := a.remapURL(fileURL) if err != nil { return nil, err } - fileAsset.FileURL = normalizedFile - glog.V(4).Infof("adding remapped file: %q", fileAsset.FileURL.String()) + fileAsset.DownloadURL = normalizedFile + glog.V(4).Infof("adding remapped file: %q", fileAsset.DownloadURL.String()) } a.FileAssets = append(a.FileAssets, fileAsset) - return fileAsset.FileURL, nil + return fileAsset.DownloadURL, nil } // FindHash returns the hash value of a FileAsset. @@ -281,9 +281,9 @@ func (a *AssetBuilder) findHash(file *FileAsset) (*hashing.Hash, error) { // TLDR; we use the file.CanonicalFileURL during assets phase, and use file.FileUrl the // rest of the time. If not we get a chicken and the egg problem where we are reading the sha file // before it exists. - u := file.FileURL - if a.Phase == "assets" && file.CanonicalFileURL != nil { - u = file.CanonicalFileURL + u := file.DownloadURL + if a.Phase == "assets" && file.CanonicalURL != nil { + u = file.CanonicalURL } if u == nil { @@ -311,24 +311,21 @@ func (a *AssetBuilder) findHash(file *FileAsset) (*hashing.Hash, error) { return nil, fmt.Errorf("cannot determine hash for %q (have you specified a valid file location?)", u) } -func (a *AssetBuilder) normalizeURL(file *url.URL) (*url.URL, error) { - - if a.AssetsLocation == nil || a.AssetsLocation.FileRepository == nil { - return nil, fmt.Errorf("assetLocation and fileRepository cannot be nil to normalize an file asset URL") +func (a *AssetBuilder) remapURL(canonicalURL *url.URL) (*url.URL, error) { + f := "" + if a.AssetsLocation != nil { + f = values.StringValue(a.AssetsLocation.FileRepository) } - - f := values.StringValue(a.AssetsLocation.FileRepository) - if f == "" { - return nil, fmt.Errorf("assetsLocation fileRepository cannot be an empty string") + return nil, fmt.Errorf("assetsLocation.fileRepository must be set to remap asset %v", canonicalURL) } fileRepo, err := url.Parse(f) if err != nil { - return nil, fmt.Errorf("unable to parse file repository URL %q: %v", values.StringValue(a.AssetsLocation.FileRepository), err) + return nil, fmt.Errorf("unable to parse assetsLocation.fileRepository %q: %v", f, err) } - fileRepo.Path = path.Join(fileRepo.Path, file.Path) + fileRepo.Path = path.Join(fileRepo.Path, canonicalURL.Path) return fileRepo, nil } diff --git a/upup/pkg/fi/cloudup/loader.go b/upup/pkg/fi/cloudup/loader.go index b396b31120..f94409877c 100644 --- a/upup/pkg/fi/cloudup/loader.go +++ b/upup/pkg/fi/cloudup/loader.go @@ -227,23 +227,23 @@ func (l *Loader) addAssetCopyTasks(assets []*assets.ContainerAsset, lifecycle *f func (l *Loader) addAssetFileCopyTasks(assets []*assets.FileAsset, lifecycle *fi.Lifecycle) error { for _, asset := range assets { - if asset.FileURL == nil { + if asset.DownloadURL == nil { return fmt.Errorf("asset file url cannot be nil") } // test if the asset needs to be copied - if asset.CanonicalFileURL != nil && asset.FileURL.String() != asset.CanonicalFileURL.String() { - glog.V(10).Infof("processing asset: %q, %q", asset.FileURL.String(), asset.CanonicalFileURL.String()) + if asset.CanonicalURL != nil && asset.DownloadURL.String() != asset.CanonicalURL.String() { + glog.V(10).Infof("processing asset: %q, %q", asset.DownloadURL.String(), asset.CanonicalURL.String()) context := &fi.ModelBuilderContext{ Tasks: l.tasks, } - glog.V(10).Infof("adding task: %q", asset.FileURL.String()) + glog.V(10).Infof("adding task: %q", asset.DownloadURL.String()) copyFileTask := &assettasks.CopyFile{ - Name: fi.String(asset.CanonicalFileURL.String()), - TargetFile: fi.String(asset.FileURL.String()), - SourceFile: fi.String(asset.CanonicalFileURL.String()), + Name: fi.String(asset.CanonicalURL.String()), + TargetFile: fi.String(asset.DownloadURL.String()), + SourceFile: fi.String(asset.CanonicalURL.String()), SHA: fi.String(asset.SHAValue), Lifecycle: lifecycle, } diff --git a/upup/pkg/fi/dryrun_target.go b/upup/pkg/fi/dryrun_target.go index 4738e2529a..b0bb3ddf1d 100644 --- a/upup/pkg/fi/dryrun_target.go +++ b/upup/pkg/fi/dryrun_target.go @@ -261,10 +261,10 @@ func (t *DryRunTarget) PrintReport(taskMap map[string]Task, out io.Writer) error if len(t.assetBuilder.FileAssets) != 0 { glog.V(4).Infof("FileAssets:") for _, a := range t.assetBuilder.FileAssets { - if a.FileURL != nil && a.CanonicalFileURL != nil { - glog.V(4).Infof(" %s %s", a.FileURL.String(), a.CanonicalFileURL.String()) - } else if a.FileURL != nil { - glog.V(4).Infof(" %s", a.FileURL.String()) + if a.DownloadURL != nil && a.CanonicalURL != nil { + glog.V(4).Infof(" %s %s", a.DownloadURL.String(), a.CanonicalURL.String()) + } else if a.DownloadURL != nil { + glog.V(4).Infof(" %s", a.DownloadURL.String()) } } }