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

Make checksum func public #252

Merged
merged 3 commits into from
May 18, 2020
Merged

Make checksum func public #252

merged 3 commits into from
May 18, 2020

Conversation

azr
Copy link
Contributor

@azr azr commented May 14, 2020

Hello there, we needed that functionality as a standalone one in Packer as we have some internal code that uploads artifacts to a remote instance and we then need to checksum it.

We could have copied it since all the fields of FileChecksum are public but I thought this tiny improvement would be a little better.

Copy link

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

LGTM 👍

checksum.go Outdated
@@ -67,7 +75,7 @@ func (c *FileChecksum) checksum(source string) error {
Hash: c.Hash,
Actual: actual,
Expected: c.Value,
File: source,
File: file,
Copy link

Choose a reason for hiding this comment

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

This would return a closed file. Is that intended?

Copy link

Choose a reason for hiding this comment

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

Oh, I misread, file isn't the file it's the path.

Choose a reason for hiding this comment

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

Would it be better to rename the variable then?

Copy link
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 think that should probably be filePath !

Copy link

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Makes sense!

@azr
Copy link
Contributor Author

azr commented May 18, 2020

Thanks everyone ! 🙂

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.

6 participants