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

HMS-5390: omit null values in upload response #953

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

xbhouse
Copy link
Contributor

@xbhouse xbhouse commented Jan 24, 2025

Summary

  • Omits null values for created, last_updated, artifact_href, and completed_checksums from the upload response (used when creating an upload or uploading a chunk). These values are not always needed and null values here prevent the generated IQE client from calling these APIs
  • Adds the upload uuid to the response when an artifact href is found (i'm not sure this should be null or omitted)

Testing steps

  1. Responses from creating an upload and uploading a chunk shouldn't have any null values
  2. Uploading rpms in the UI should still work the same (existing uploads should be reused)
  3. IQE: Go through the uploading flow using the IQE environment, let me know if there are any other API type errors etc and I'll make the changes here

@jlsherrill
Copy link
Member

@xbhouse xbhouse marked this pull request as ready for review January 24, 2025 21:40
@xbhouse xbhouse marked this pull request as draft January 24, 2025 21:43
@mayurilahane
Copy link
Contributor

/retest

@mayurilahane
Copy link
Contributor

/retest

@xbhouse xbhouse marked this pull request as ready for review January 29, 2025 16:02
@mayurilahane
Copy link
Contributor

/retest

@mayurilahane
Copy link
Contributor

null values in upload response is fixed and able to upload the chunk of rpm file

@mayurilahane
Copy link
Contributor

ACK

Copy link
Contributor

@dominikvagner dominikvagner left a comment

Choose a reason for hiding this comment

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

one small comment, works and looks good! 👏🏼😄

Comment on lines 712 to 713
ArtifactHref: utils.Ptr(""),
CompletedChecksums: make([]string, 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason for doing this rather than also adding the omitempty option, like for the created and last_updated?
I think that would result in cleaner API responses 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep you're right, it would be cleaner! the iqe test is checking the artifact_href, but i think we could instead check if this is in the response at all instead of checking it's empty. i'll push those changes up and verify the test changes we'll need

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed those up and @mayurilahane verified the changes in her upload test are working with this 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome, nice job 🎉 👏🏼
approved ✅

@dominikvagner dominikvagner self-assigned this Jan 30, 2025
@dominikvagner
Copy link
Contributor

/retest

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.

4 participants