Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [PIPE-20976]: Refactor Downloader and Add UTs #11

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

vcalasansh
Copy link
Contributor

@vcalasansh vcalasansh commented Sep 19, 2024

With the intent of improving code quality of the download package, in this PR we refactor it (renaming it to downloader). The Downloader struct becomes like this:

type Downloader struct {
	dir                  string
	repoDownloader       *repoDownloader
	executableDownloader *executableDownloader
}

So that instead of having a the Downloader provide a single Download method, we make it provide 2 distinct methods, one for cloning repositories, and another for downloading executable files:

// for cloning repositories
func (d *Downloader) DownloadRepo(ctx context.Context, repo *task.Repository) (string, error)

// for downloading an executable file
func (d *Downloader) DownloadExecutable(ctx context.Context, taskType string, version string, exec *task.ExecutableConfig) (string, error)

In this way, we let the caller figure out which kind of download it wants to carry out, and call the appropriate method. We can then avoid the Downloader class containing mixed logics for cloning repositories and downloading executables, thus improving maintainability of the codebase.

We have also moved the common functionality, used by both repoDownloader and executableDownloader, to util.go file, and added UTs for files in the downloader package.

TESTING:
All tests in the repo are passing. CGI tasks are also working fine if you change the Task structure to use json.RawMessage instead of []byte:

type Task struct {
       ....

	// Data provides task execution data.
	Data json.RawMessage `json:"data"`
        
        ....

	// Config provides the execution driver configuration.
	Config json.RawMessage `json:"config"`

       ....
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant