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

PRO-752: Calculate package checksum locally when downloaded #66

Merged
merged 9 commits into from
Dec 11, 2023

Conversation

BartoszBlizniak
Copy link
Contributor

Previously, we were grabbing the package checksum via our API which meant that the checksum could be out-of-sync whenever a package is re-published due to caching. This commit ensures that the checksums that are being returned are from the downloaded package and when the package is not downloaded, the checksums will then be returned from the API.

@BartoszBlizniak BartoszBlizniak requested a review from a team as a code owner November 23, 2023 13:44
Copy link
Member

@lskillen lskillen left a comment

Choose a reason for hiding this comment

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

This is really awesome; I think we need a little bit extra to make this safe.

We should also really add an additional test/coverage for the branches (and new logic) here as well to cloudsmith/data_source_package_test.go - creating more work for you Bart, sorry, but better to be safe! 🙏

Happy to get you some help and extra eyes if wanted. 😁

Copy link
Contributor

@dgonzalez dgonzalez left a comment

Choose a reason for hiding this comment

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

Aside from Lee's comment, a nit pick that you can ignore but looks good from my side.

cloudsmith/data_source_package.go Outdated Show resolved Hide resolved
@BartoszBlizniak
Copy link
Contributor Author

Hey, I've added the logic for re-try and updated the test, could I get another 👀

Copy link
Member

@lskillen lskillen left a comment

Choose a reason for hiding this comment

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

Few wee comments; main one is to force a failure instead of a warning. :)

cloudsmith/data_source_package.go Outdated Show resolved Hide resolved
cloudsmith/data_source_package.go Outdated Show resolved Hide resolved
cloudsmith/data_source_package_test.go Show resolved Hide resolved
@BartoszBlizniak
Copy link
Contributor Author

Hey @lskillen - are we okay to release this or are there some other things that I should change here?

Copy link

@estebangarcia estebangarcia left a comment

Choose a reason for hiding this comment

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

I might be late to the party but have a couple of suggestions.

cloudsmith/data_source_package.go Outdated Show resolved Hide resolved
cloudsmith/data_source_package.go Outdated Show resolved Hide resolved
cloudsmith/data_source_package.go Outdated Show resolved Hide resolved
@lskillen
Copy link
Member

lskillen commented Dec 8, 2023

I might be late to the party but have a couple of suggestions.

These are more or less the same feedback for me, but I was thinking of a live share with Bart and we can get this over the line; do you fancy doing a VSCode Live Share session with Bart, @estebangarcia? I reckon you two could knock it out pretty quickly!

@BartoszBlizniak
Copy link
Contributor Author

Would be happy to! Just throw something into my calendar when suits the best :)

@BartoszBlizniak
Copy link
Contributor Author

Thanks @estebangarcia for the session! @lskillen - if you'd like to glance over final code and I can get it shipped out

@lskillen
Copy link
Member

lskillen commented Dec 11, 2023

Looks sane to me 👍 ... except for the use of tabs everywhere, which is Not Your Fault.

Something something eternal holy war. 😂 (tbh, I'm not precious about it, but we're spaces everywhere else!)

@BartoszBlizniak
Copy link
Contributor Author

Looks sane to me 👍 ... except for the use of tabs everywhere, which is Not Your Fault.

Something something eternal holy war. 😂 (tbh, I'm not precious about it, but we're spaces everywhere else!)

tabs > spaces

@BartoszBlizniak BartoszBlizniak merged commit 8392c42 into master Dec 11, 2023
4 checks passed
@BartoszBlizniak BartoszBlizniak deleted the PRO-752/check_checksum_package_locally branch December 11, 2023 17:28
@lskillen
Copy link
Member

Looks sane to me 👍 ... except for the use of tabs everywhere, which is Not Your Fault.
Something something eternal holy war. 😂 (tbh, I'm not precious about it, but we're spaces everywhere else!)

tabs > spaces

(I know tabs are the default for go fmt and friends)

OK, but you can rewrite the rest of the codebase using tabs. :p

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

Successfully merging this pull request may close these issues.

None yet

4 participants