Skip to content

Facilitate incremental download of packages#1331

Merged
mdrohmann merged 2 commits intomasterfrom
martind/incremental-download-177290892
Apr 29, 2021
Merged

Facilitate incremental download of packages#1331
mdrohmann merged 2 commits intomasterfrom
martind/incremental-download-177290892

Conversation

@mdrohmann
Copy link
Copy Markdown
Contributor

@mdrohmann mdrohmann commented Apr 29, 2021

Naatan
Naatan previously approved these changes Apr 29, 2021
Copy link
Copy Markdown
Contributor

@Naatan Naatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me. Did you consider how this affects the env merging at the end?

Comment on lines +28 to +29
func (s *Setup) DeleteOutdatedArtifacts(changeset artifact.ArtifactChangeset, storedArtifacted store.StoredArtifactMap) ([]artifact.ArtifactID, error) {
var alreadyInstalled []artifact.ArtifactID
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not going to request changes on this now, but this seems like something we should fix in the future. It's awkward that a function responsible for deleting outdated artifacts is returning artifacts that you want to keep. Either this function needs to be renamed or its logic needs to be split up into separate functions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I pushed a new version, that also gets rid of the go-funk dependency...

Copy link
Copy Markdown
Contributor

@Naatan Naatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!

@mdrohmann mdrohmann merged commit 16f1947 into master Apr 29, 2021
@mdrohmann mdrohmann deleted the martind/incremental-download-177290892 branch April 29, 2021 20:47
Naatan pushed a commit that referenced this pull request May 6, 2021
…d-177290892

Facilitate incremental download of packages
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.

2 participants