From f80d841188d57afaadfe73d405de3527b900cbfa Mon Sep 17 00:00:00 2001 From: mmetc <92726601+mmetc@users.noreply.github.com> Date: Thu, 9 Nov 2023 12:07:09 +0100 Subject: [PATCH] Refact cwhub: make some methods private (#2584) * make hub.enableItem() private * make hub.downloadLatest() private * make getData() private * make hub.disableItem() private * make hub.downloadItem() private * make hub.syncDir() private * make hub.localSync() private; keep warnings in Hub struct (no need to call LocalSync to get them) --- cmd/crowdsec-cli/hub.go | 8 ++------ pkg/cwhub/dataset.go | 4 ++-- pkg/cwhub/enable.go | 13 ++++++------- pkg/cwhub/enable_test.go | 26 +++++++++++++------------- pkg/cwhub/helpers.go | 30 +++++++++++++++--------------- pkg/cwhub/hub.go | 3 ++- pkg/cwhub/sync.go | 19 ++++++++++++------- 7 files changed, 52 insertions(+), 51 deletions(-) diff --git a/cmd/crowdsec-cli/hub.go b/cmd/crowdsec-cli/hub.go index abf99175a..f934bd856 100644 --- a/cmd/crowdsec-cli/hub.go +++ b/cmd/crowdsec-cli/hub.go @@ -53,9 +53,7 @@ func runHubList(cmd *cobra.Command, args []string) error { return err } - // use LocalSync to get warnings about tainted / outdated items - warn, _ := hub.LocalSync() - for _, v := range warn { + for _, v := range hub.Warnings { log.Info(v) } @@ -96,9 +94,7 @@ func runHubUpdate(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to update hub: %w", err) } - // use LocalSync to get warnings about tainted / outdated items - warn, _ := hub.LocalSync() - for _, v := range warn { + for _, v := range hub.Warnings { log.Info(v) } diff --git a/pkg/cwhub/dataset.go b/pkg/cwhub/dataset.go index d57827c3e..2b3629a97 100644 --- a/pkg/cwhub/dataset.go +++ b/pkg/cwhub/dataset.go @@ -54,7 +54,7 @@ func downloadFile(url string, destPath string) error { return nil } -func GetData(data []types.DataSource, dataDir string) error { +func getData(data []types.DataSource, dataDir string) error { for _, dataS := range data { destPath := filepath.Join(dataDir, dataS.DestPath) log.Infof("downloading data '%s' in '%s'", dataS.SourceURL, destPath) @@ -91,7 +91,7 @@ func downloadData(dataFolder string, force bool, reader io.Reader) error { } if download || force { - if err := GetData(data.Data, dataFolder); err != nil { + if err := getData(data.Data, dataFolder); err != nil { return fmt.Errorf("while getting data: %w", err) } } diff --git a/pkg/cwhub/enable.go b/pkg/cwhub/enable.go index f2281bb1f..2f137fa76 100644 --- a/pkg/cwhub/enable.go +++ b/pkg/cwhub/enable.go @@ -10,10 +10,9 @@ import ( log "github.com/sirupsen/logrus" ) -// EnableItem creates a symlink between actual config file at hub.HubDir and hub.ConfigDir +// enableItem creates a symlink between actual config file at hub.HubDir and hub.ConfigDir // Handles collections recursively -// XXX: called from config_restore otherwise no need to export -func (h *Hub) EnableItem(target *Item) error { +func (h *Hub) enableItem(target *Item) error { parentDir := filepath.Clean(h.local.InstallDir + "/" + target.Type + "/" + target.Stage + "/") // create directories if needed @@ -48,7 +47,7 @@ func (h *Hub) EnableItem(target *Item) error { return fmt.Errorf("required %s %s of %s doesn't exist, abort", sub.Type, sub.Name, target.Name) } - if err := h.EnableItem(&val); err != nil { + if err := h.enableItem(&val); err != nil { return fmt.Errorf("while installing %s: %w", sub.Name, err) } } @@ -96,8 +95,8 @@ func (h *Hub) purgeItem(target Item) (Item, error) { return target, nil } -// DisableItem to disable an item managed by the hub, removes the symlink if purge is true -func (h *Hub) DisableItem(target *Item, purge bool, force bool) error { +// disableItem to disable an item managed by the hub, removes the symlink if purge is true +func (h *Hub) disableItem(target *Item, purge bool, force bool) error { // XXX: should return the number of disabled/purged items to inform the upper layer whether to reload or not var err error @@ -141,7 +140,7 @@ func (h *Hub) DisableItem(target *Item, purge bool, force bool) error { } if toRemove { - if err = h.DisableItem(&val, purge, force); err != nil { + if err = h.disableItem(&val, purge, force); err != nil { return fmt.Errorf("while disabling %s: %w", sub.Name, err) } } else { diff --git a/pkg/cwhub/enable_test.go b/pkg/cwhub/enable_test.go index 8b7cac88d..a476c76d7 100644 --- a/pkg/cwhub/enable_test.go +++ b/pkg/cwhub/enable_test.go @@ -10,20 +10,20 @@ import ( func testInstall(hub *Hub, t *testing.T, item Item) { // Install the parser - err := hub.DownloadLatest(&item, false, false) + err := hub.downloadLatest(&item, false, false) require.NoError(t, err, "failed to download %s", item.Name) - _, err = hub.LocalSync() + err = hub.localSync() require.NoError(t, err, "failed to run localSync") assert.True(t, hub.Items[item.Type][item.Name].UpToDate, "%s should be up-to-date", item.Name) assert.False(t, hub.Items[item.Type][item.Name].Installed, "%s should not be installed", item.Name) assert.False(t, hub.Items[item.Type][item.Name].Tainted, "%s should not be tainted", item.Name) - err = hub.EnableItem(&item) + err = hub.enableItem(&item) require.NoError(t, err, "failed to enable %s", item.Name) - _, err = hub.LocalSync() + err = hub.localSync() require.NoError(t, err, "failed to run localSync") assert.True(t, hub.Items[item.Type][item.Name].Installed, "%s should be installed", item.Name) @@ -41,7 +41,7 @@ func testTaint(hub *Hub, t *testing.T, item Item) { require.NoError(t, err, "failed to write to %s (%s)", item.LocalPath, item.Name) // Local sync and check status - _, err = hub.LocalSync() + err = hub.localSync() require.NoError(t, err, "failed to run localSync") assert.True(t, hub.Items[item.Type][item.Name].Tainted, "%s should be tainted", item.Name) @@ -51,11 +51,11 @@ func testUpdate(hub *Hub, t *testing.T, item Item) { assert.False(t, hub.Items[item.Type][item.Name].UpToDate, "%s should not be up-to-date", item.Name) // Update it + check status - err := hub.DownloadLatest(&item, true, true) + err := hub.downloadLatest(&item, true, true) require.NoError(t, err, "failed to update %s", item.Name) // Local sync and check status - _, err = hub.LocalSync() + err = hub.localSync() require.NoError(t, err, "failed to run localSync") assert.True(t, hub.Items[item.Type][item.Name].UpToDate, "%s should be up-to-date", item.Name) @@ -66,26 +66,26 @@ func testDisable(hub *Hub, t *testing.T, item Item) { assert.True(t, hub.Items[item.Type][item.Name].Installed, "%s should be installed", item.Name) // Remove - err := hub.DisableItem(&item, false, false) + err := hub.disableItem(&item, false, false) require.NoError(t, err, "failed to disable %s", item.Name) // Local sync and check status - warns, err := hub.LocalSync() + err = hub.localSync() require.NoError(t, err, "failed to run localSync") - require.Empty(t, warns, "unexpected warnings: %+v", warns) + require.Empty(t, hub.Warnings) assert.False(t, hub.Items[item.Type][item.Name].Tainted, "%s should not be tainted anymore", item.Name) assert.False(t, hub.Items[item.Type][item.Name].Installed, "%s should not be installed anymore", item.Name) assert.True(t, hub.Items[item.Type][item.Name].Downloaded, "%s should still be downloaded", item.Name) // Purge - err = hub.DisableItem(&item, true, false) + err = hub.disableItem(&item, true, false) require.NoError(t, err, "failed to purge %s", item.Name) // Local sync and check status - warns, err = hub.LocalSync() + err = hub.localSync() require.NoError(t, err, "failed to run localSync") - require.Empty(t, warns, "unexpected warnings: %+v", warns) + require.Empty(t, hub.Warnings) assert.False(t, hub.Items[item.Type][item.Name].Installed, "%s should not be installed anymore", item.Name) assert.False(t, hub.Items[item.Type][item.Name].Downloaded, "%s should not be downloaded", item.Name) diff --git a/pkg/cwhub/helpers.go b/pkg/cwhub/helpers.go index 89e9c747c..e8b49b814 100644 --- a/pkg/cwhub/helpers.go +++ b/pkg/cwhub/helpers.go @@ -35,7 +35,7 @@ func (h *Hub) InstallItem(name string, itemType string, force bool, downloadOnly } // XXX: confusing semantic between force and updateOnly? - if err := h.DownloadLatest(item, force, true); err != nil { + if err := h.downloadLatest(item, force, true); err != nil { return fmt.Errorf("while downloading %s: %w", item.Name, err) } @@ -44,14 +44,14 @@ func (h *Hub) InstallItem(name string, itemType string, force bool, downloadOnly } if downloadOnly { - // XXX: should get the path from DownloadLatest + // XXX: should get the path from downloadLatest log.Infof("Downloaded %s to %s", item.Name, filepath.Join(h.local.HubDir, item.RemotePath)) return nil } // XXX: should we stop here if the item is already installed? - if err := h.EnableItem(item); err != nil { + if err := h.enableItem(item); err != nil { return fmt.Errorf("while enabling %s: %w", item.Name, err) } @@ -83,11 +83,11 @@ func (h *Hub) RemoveItem(itemType string, name string, purge bool, forceAction b return false, nil } - if err := h.DisableItem(item, purge, forceAction); err != nil { + if err := h.disableItem(item, purge, forceAction); err != nil { return false, fmt.Errorf("unable to disable %s: %w", item.Name, err) } - // XXX: should take the value from DisableItem + // XXX: should take the value from disableItem removed = true if err := h.AddItem(*item); err != nil { @@ -127,7 +127,7 @@ func (h *Hub) UpgradeItem(itemType string, name string, force bool) (bool, error } } - if err := h.DownloadLatest(item, force, true); err != nil { + if err := h.downloadLatest(item, force, true); err != nil { return false, fmt.Errorf("%s: download failed: %w", item.Name, err) } @@ -153,9 +153,9 @@ func (h *Hub) UpgradeItem(itemType string, name string, force bool) (bool, error return updated, nil } -// DownloadLatest will download the latest version of Item to the tdir directory -func (h *Hub) DownloadLatest(target *Item, overwrite bool, updateOnly bool) error { - // XXX: should return the path of the downloaded file (taken from DownloadItem) +// downloadLatest will download the latest version of Item to the tdir directory +func (h *Hub) downloadLatest(target *Item, overwrite bool, updateOnly bool) error { + // XXX: should return the path of the downloaded file (taken from downloadItem) log.Debugf("Downloading %s %s", target.Type, target.Name) if !target.HasSubItems() { @@ -165,7 +165,7 @@ func (h *Hub) DownloadLatest(target *Item, overwrite bool, updateOnly bool) erro } // XXX: - return h.DownloadItem(target, overwrite) + return h.downloadItem(target, overwrite) } // collection @@ -186,21 +186,21 @@ func (h *Hub) DownloadLatest(target *Item, overwrite bool, updateOnly bool) erro if sub.HasSubItems() { log.Tracef("collection, recurse") - if err := h.DownloadLatest(&val, overwrite, updateOnly); err != nil { + if err := h.downloadLatest(&val, overwrite, updateOnly); err != nil { return fmt.Errorf("while downloading %s: %w", val.Name, err) } } downloaded := val.Downloaded - if err := h.DownloadItem(&val, overwrite); err != nil { + if err := h.downloadItem(&val, overwrite); err != nil { return fmt.Errorf("while downloading %s: %w", val.Name, err) } // We need to enable an item when it has been added to a collection since latest release of the collection. // We check if val.Downloaded is false because maybe the item has been disabled by the user. if !val.Installed && !downloaded { - if err := h.EnableItem(&val); err != nil { + if err := h.enableItem(&val); err != nil { return fmt.Errorf("enabling '%s': %w", val.Name, err) } } @@ -208,14 +208,14 @@ func (h *Hub) DownloadLatest(target *Item, overwrite bool, updateOnly bool) erro h.Items[sub.Type][sub.Name] = val } - if err := h.DownloadItem(target, overwrite); err != nil { + if err := h.downloadItem(target, overwrite); err != nil { return fmt.Errorf("failed to download item: %w", err) } return nil } -func (h *Hub) DownloadItem(target *Item, overwrite bool) error { +func (h *Hub) downloadItem(target *Item, overwrite bool) error { url, err := h.remote.urlTo(target.RemotePath) if err != nil { return fmt.Errorf("failed to build hub item request: %w", err) diff --git a/pkg/cwhub/hub.go b/pkg/cwhub/hub.go index 823ede257..f2b6c4b88 100644 --- a/pkg/cwhub/hub.go +++ b/pkg/cwhub/hub.go @@ -17,6 +17,7 @@ type Hub struct { remote *RemoteHubCfg skippedLocal int skippedTainted int + Warnings []string } var theHub *Hub @@ -56,7 +57,7 @@ func NewHub(local *csconfig.LocalHubCfg, remote *RemoteHubCfg, downloadIndex boo return nil, fmt.Errorf("failed to load index: %w", err) } - if _, err := theHub.LocalSync(); err != nil { + if err := theHub.localSync(); err != nil { return nil, fmt.Errorf("failed to sync items: %w", err) } diff --git a/pkg/cwhub/sync.go b/pkg/cwhub/sync.go index 6193bf388..e9ad1b41c 100644 --- a/pkg/cwhub/sync.go +++ b/pkg/cwhub/sync.go @@ -391,7 +391,7 @@ func (h *Hub) checkSubItems(v *Item) error { return nil } -func (h *Hub) SyncDir(dir string) ([]string, error) { +func (h *Hub) syncDir(dir string) ([]string, error) { warnings := []string{} // For each, scan PARSERS, POSTOVERFLOWS, SCENARIOS and COLLECTIONS last @@ -439,18 +439,23 @@ func (h *Hub) SyncDir(dir string) ([]string, error) { } // Updates the info from HubInit() with the local state -func (h *Hub) LocalSync() ([]string, error) { +func (h *Hub) localSync() error { h.skippedLocal = 0 h.skippedTainted = 0 + h.Warnings = []string{} - warnings, err := h.SyncDir(h.local.InstallDir) + warnings, err := h.syncDir(h.local.InstallDir) if err != nil { - return warnings, fmt.Errorf("failed to scan %s: %w", h.local.InstallDir, err) + return fmt.Errorf("failed to scan %s: %w", h.local.InstallDir, err) } - if _, err = h.SyncDir(h.local.HubDir); err != nil { - return warnings, fmt.Errorf("failed to scan %s: %w", h.local.HubDir, err) + h.Warnings = append(h.Warnings, warnings...) + + if warnings, err = h.syncDir(h.local.HubDir); err != nil { + return fmt.Errorf("failed to scan %s: %w", h.local.HubDir, err) } - return warnings, nil + h.Warnings = append(h.Warnings, warnings...) + + return nil }