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

Switch description from query parameter to form data sending deploy description to dag server #1726

Merged
merged 11 commits into from
Oct 14, 2024

Conversation

Simpcyclassy
Copy link
Member

@Simpcyclassy Simpcyclassy commented Oct 3, 2024

Description

This PR modifies the way the description is handled in the UploadFile function. Previously, the description was passed as a query parameter in the URL. It is now switched to be sent as form data along with the file in the multipart request.

Key changes:

  • Switched description from a query parameter to form data.
  • Added the description field to the UploadFileArguments struct.
  • Updated the UploadFile function to handle the description as form data.

This change improves the handling of spaces and special characters in the description, avoiding issues caused by URL encoding.

Related astronomer/issues/issues/6663

🧪 Functional Testing

  • Adjusted all related test cases in TestUploadFile to reflect this change.

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@pgvishnuram pgvishnuram marked this pull request as ready for review October 3, 2024 18:28
@@ -814,6 +820,7 @@ func (s *Suite) TestUploadFile() {
TargetURL: server.URL,
FormFileFieldName: "file",
Headers: headers,
Description: "This is a test description",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this test case to check the contents of the request, to verify the file contents and the description are in there?

@@ -632,6 +632,7 @@ func (s *Suite) TestUploadFile() {
TargetURL: "http://localhost:8080/upload",
FormFileFieldName: "file",
Headers: map[string]string{},
Description: "This is a test description",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also include a test with the description set to an empty string and Jeremy's comment to ensure regression is intact.

req.Header.Set(k, v)
}
setHeaders(req, headers)

client := &http.Client{}
response, err := client.Do(req)
Copy link
Member Author

@Simpcyclassy Simpcyclassy Oct 4, 2024

Choose a reason for hiding this comment

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

I only tested for no description. I was unable to fully test the description. When logging the request body just before and after this line response, err := client.Do(req), I noticed that no data is logged after the request is sent.
Given this, simulating and testing for the description field and even file did not quite work.

Copy link
Contributor

@neel-astro neel-astro left a comment

Choose a reason for hiding this comment

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

Bringing in @lzdanski's comment from the docs PR for release changes, could we update the description of the newly added description flag, to match the description added in docs (here) for consistency.

@Simpcyclassy
Copy link
Member Author

I do not have the permission to merge. I need help merging this @neel-astro

@neel-astro neel-astro merged commit 920e82c into main Oct 14, 2024
3 checks passed
@neel-astro neel-astro deleted the add-description-to-uploadfile-formdata branch October 14, 2024 10:45
neel-astro added a commit that referenced this pull request Oct 15, 2024
…escription to dag server (#1726)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pgvishnuram <vishnu@astronomer.io>
Co-authored-by: Neel Dalsania <neel.dalsania@astronomer.io>
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.

4 participants