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

Accept progress on numbers >2GB #43328

Conversation

kgb-financial-com
Copy link
Contributor

@kgb-financial-com kgb-financial-com commented Nov 29, 2024

Docker ProgressUpdateEvent may report file sizes larger than 2GB, but int has been used as the target type on parsing the according json output. If we attempt to push an image with a total size of >2GB, this will result in a build failure (although the image will still be pushed correctly).

This Pull Request fixes that by changing the affected types from int to long.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 29, 2024
@wilkinsona
Copy link
Member

@kgb-financial-com Thanks for the PR. Unfortunately, this doesn't qualify as an obvious fix as it changes functionality. Please sign the CLA.

@kgb-financial-com
Copy link
Contributor Author

Thanks. Please give me a few days to get approval from our top brass.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Nov 30, 2024
This fix is needed to push images of a file size >2GB
without provoking build failures.
@pivotal-cla
Copy link

@kgb-financial-com Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 2, 2024
@pivotal-cla
Copy link

@kgb-financial-com Thank you for signing the Contributor License Agreement!

@kgb-financial-com
Copy link
Contributor Author

I have removed the tag as obvious fix from the original pull request description, and signed the CLA. Please proceed. :)

@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Dec 2, 2024
@wilkinsona
Copy link
Member

Thanks for signing the CLA.

Technically speaking, this is a breaking change as the constructor of ProgressDetail and the two getter methods are public API. In reality, I think it's likely that the API isn't used publicly so I'd be comfortable applying the change in 3.5 but I'm slightly wary of applying it in 3.3.x and 3.4.x. We'll discuss it as a team to figure out the next steps.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Dec 2, 2024
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 2, 2024
@philwebb philwebb added this to the 3.3.x milestone Dec 2, 2024
@philwebb philwebb self-assigned this Dec 2, 2024
@philwebb philwebb modified the milestones: 3.3.x, 3.3.7 Dec 2, 2024
@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Dec 2, 2024
philwebb pushed a commit that referenced this pull request Dec 2, 2024
Update `ProgressUpdateEvent` to support images of a file size
>2GB without provoking build failures.

See gh-43328
philwebb added a commit that referenced this pull request Dec 2, 2024
Restore `int` returns for existing methods and deprecate them in
favor of a new `asPercentage()` method.

See gh-43328
@philwebb philwebb closed this in 5a8e3d5 Dec 2, 2024
@philwebb
Copy link
Member

philwebb commented Dec 2, 2024

Looking at our own code, we only have one place that we call those methods which we can replace with a new asPercentage() method. I've polished the contribution to do that, and to restore the int returns in a deprecated form.

I agree with @wilkinsona that this class is mostly internal so I think we can be aggressive about removing the deprecated methods in 3.5.

Thanks for the contribution @kgb-financial-com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants