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

chore(file domain): add network.DownloadFile and use go-getter to download files #760

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

Conversation

mildwonkey
Copy link
Contributor

Description

This PR adds a new method, network.DownloadFile, which users go-getter to download files. It's currently in use by the file domain. Go-getter's Get supports local files, network downloads (network.Fetch also already handles that!) and also a number of protocols such as ftp, s3, git, etc.

I limited the functionality to downloading INDIVIDUAL FILES and not full directories and repositories for now, since it would take more testing and a slightly larger refactor of the file domain. (I have an idea for that, but wanted to get this PR dealt with first). A future PR will add network.Download for directories, git repos, etc. (This will require an additional field in the file spec, source or something similar. Users will still need to list the files that the validations run against).

I added one test that pulls a file from our git repo to the e2e tests. There is a comment in the file warning future-us of that, but it's still worth mentioning that will break and annoy us some day (and be easily fixed, but still. Annoying).

There are some other changes in the e2e test files - It was getting hard to find the actual test output, so I removed all the calls to message.Info. I also noticed I had randomly started using assert where I wanted require in the files domain e2e tests, so that's fixed too 🙄

...

Related Issue

Relates to #693

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)
    more of a precursor to a new feature, but close enough?

@mildwonkey mildwonkey requested a review from a team as a code owner October 24, 2024 15:09
@mildwonkey mildwonkey changed the title chore(file domain): add network.DownloadFile and use go-getter to download files chore(file domain): add network.DownloadFile and use go-getter to download files Oct 24, 2024
@meganwolf0
Copy link
Collaborator

meganwolf0 commented Oct 25, 2024

Is the intent to completely move off network.Fetch, or is this supposed to just be like a separate functionality with the intent to support more specialized downloads specifically for the file domain?

Also, because I was just at a security conference and it's fresh, looking at like the dependencies for the current network package (which are all go built-ins) vs this PR which adds some super old/forked package deps (I'm sure we have a ton of that, honestly this is really the first time I'm really looking under the hood there), just thinking more about our posture on these kinds of things/potentially accepting risks. More of a team discussion I think though.


Edit after thinking about the security comment a bit more -
Maybe an alternative solution would be to incorporate the go-getter into the files domain instead of pkg, because it definitely has the capacity to support better use cases than just simple file download, but also maintain network.Fetch in our pkg/core Lula features since that is really only used for compose and I don't really see much benefit to swapping to go getter (maybe there is, I suppose for the auth case if we have to auth to compose... sidebar since that's also a can of worms with specifying where that data would be). Then if we wanted to like more discretely break out domains in the future (for like security things), we wouldn't be strictly dependent on go getter for core lula functions.

@mildwonkey
Copy link
Contributor Author

This isn't going to replace Fetch, since go-getter only writes files and most callers just need to read the bytes. My original files domain PR just used go-getter in the domain and I don't mind doing that again.

No argument on the general dependency concern, but if we make the domains external plugins, go-getter is the library I'd suggest we use to download them. It is indeed always a risk, but at least go-getter has a ton of attention on it thanks to several projects, instead of managing it all myself or trusting a library I've never heard of with 6 stars (side note, I'd love an approved go libraries list). Having said that, if we don't feel good about this, I have no problem closing it till we can discuss more.

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

Successfully merging this pull request may close these issues.

2 participants