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

create CalculateSize() #719

Merged
merged 2 commits into from
Nov 20, 2020
Merged

create CalculateSize() #719

merged 2 commits into from
Nov 20, 2020

Conversation

deitch
Copy link
Collaborator

@deitch deitch commented May 1, 2020

We already pull out the size of the file to use when reporting updates; might as well expose it and simplify our routines.

@deitch
Copy link
Collaborator Author

deitch commented May 7, 2020

Hey @imjasonh and @jonjohnsonjr ; this is the final follow-up from those other ones, notably #675 and its predecessors.

I might start doing some of these progress and other things for the v1layout format as well, once this is in.

// given the input data. Provided by rounding up to nearest whole block (512)
// and adding header 512
func CalculateTarFileSize(in int64) (out int64) {
func CalculateSingleFileInTarSize(in int64) (out int64) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be exported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to? Not really, but it is such a nice convenience function that it might be useful.

You kind of wish this were built into core golang archive/tar package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree this is useful, but I'd like to avoid exposing anything unless it's really necessary. They might accept a commit to expose some of archive/tar? https://github.com/golang/go/blob/8f2db14cd35bbd674cb2988a508306de6655e425/src/archive/tar/format.go#L141-L152

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I will privatize it and rebase and update. Check soon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got it. Take another look. It has been a while, I might have made an error on the rebase, so please check carefully. Another pair of eyes is always helpful.

pkg/v1/tarball/write.go Show resolved Hide resolved
@deitch
Copy link
Collaborator Author

deitch commented Jun 18, 2020

Oof, it's been a strange couple months so I've let some things slip, sorry about that.

No worries.

@deitch
Copy link
Collaborator Author

deitch commented Jun 18, 2020

While we are at it, separately, would you take a "download progress updates" on Layer() and similar like we did on the entire tarball?

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@deitch
Copy link
Collaborator Author

deitch commented Oct 27, 2020

This has been open super long. Can we get it accepted and merged or denied for a reason and closed @imjasonh or @jonjohnsonjr ? Thanks

@deitch
Copy link
Collaborator Author

deitch commented Oct 27, 2020

/reopen

@deitch
Copy link
Collaborator Author

deitch commented Oct 27, 2020

/remove-lifecycle stale

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

Going through and cleaning up old issues -- I didn't realize we never merged this. If you'd still like to get it merged, it LGTM other than exposing CalculateSingleFileInTarSize, which feels out of place.

// given the input data. Provided by rounding up to nearest whole block (512)
// and adding header 512
func CalculateTarFileSize(in int64) (out int64) {
func CalculateSingleFileInTarSize(in int64) (out int64) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree this is useful, but I'd like to avoid exposing anything unless it's really necessary. They might accept a commit to expose some of archive/tar? https://github.com/golang/go/blob/8f2db14cd35bbd674cb2988a508306de6655e425/src/archive/tar/format.go#L141-L152

Signed-off-by: Avi Deitcher <avi@deitcher.net>
Co-authored-by: jonjohnsonjr <jonjohnson@google.com>
@jonjohnsonjr
Copy link
Collaborator

Looks like travis doesn't have very many windows machines available :(

@jonjohnsonjr
Copy link
Collaborator

Travis failed but I didn't notice, retried and it works.

@jonjohnsonjr jonjohnsonjr merged commit 6df0303 into google:master Nov 20, 2020
@deitch deitch deleted the tar-size branch November 21, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants