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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0506215
Add description field to UploadFile to send as form data along with t…
Simpcyclassy Oct 3, 2024
9b39ddf
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 3, 2024
16528a5
mimize code functions
pgvishnuram Oct 3, 2024
54d5b36
successfully uploaded with an empty description
Simpcyclassy Oct 4, 2024
0bc0c2b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 4, 2024
669c2b6
remove unused func
Simpcyclassy Oct 4, 2024
a267adc
Merge branch 'add-description-to-uploadfile-formdata' of https://gith…
Simpcyclassy Oct 4, 2024
23ce940
Merge branch 'main' into add-description-to-uploadfile-formdata
Simpcyclassy Oct 9, 2024
bbc20ee
remove trailing n
Simpcyclassy Oct 9, 2024
69568a6
update flag description for deploy --description
Simpcyclassy Oct 10, 2024
cdac85c
fix broken unit test logic around mock server capturing request (#1730)
neel-astro Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions pkg/fileutil/files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type UploadFileArguments struct {
TargetURL string
FormFileFieldName string
Headers map[string]string
Description string
MaxTries int
InitialDelayInMS int
BackoffFactor int
Expand Down Expand Up @@ -310,6 +311,13 @@ func UploadFile(args *UploadFileArguments) error {
if err != nil {
return fmt.Errorf("error copying file content: %w", err)
}
// Add the description field to the multipart request
if args.Description != "" {
err = writer.WriteField("description", args.Description)
if err != nil {
return fmt.Errorf("error adding description field: %w", err)
}
}
// Close the multipart writer to finalize the request
writer.Close()

Expand All @@ -326,9 +334,8 @@ func UploadFile(args *UploadFileArguments) error {
}

// set headers
for k, v := range headers {
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 •
edited
Loading

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.

if err != nil {
Expand Down Expand Up @@ -369,6 +376,12 @@ func UploadFile(args *UploadFileArguments) error {
return currentUploadError
}

func setHeaders(req *http.Request, headers map[string]string) {
for k, v := range headers {
req.Header.Set(k, v)
}
}

func GzipFile(srcFilePath, destFilePath string) error {
srcFile, err := os.Open(srcFilePath)
if err != nil {
Expand Down
7 changes: 7 additions & 0 deletions pkg/fileutil/files_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

MaxTries: 3,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand Down Expand Up @@ -660,6 +661,7 @@ func (s *Suite) TestUploadFile() {
TargetURL: "testURL",
FormFileFieldName: "file",
Headers: map[string]string{},
Description: "This is a test description",
MaxTries: 3,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand Down Expand Up @@ -688,6 +690,7 @@ func (s *Suite) TestUploadFile() {
TargetURL: "testURL",
FormFileFieldName: "file",
Headers: map[string]string{},
Description: "This is a test description",
MaxTries: 3,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand Down Expand Up @@ -721,6 +724,7 @@ func (s *Suite) TestUploadFile() {
TargetURL: testServer.URL,
FormFileFieldName: "file",
Headers: headers,
Description: "This is a test description",
MaxTries: 2,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand Down Expand Up @@ -756,6 +760,7 @@ func (s *Suite) TestUploadFile() {
TargetURL: testServer.URL,
FormFileFieldName: "file",
Headers: headers,
Description: "This is a test description",
MaxTries: 2,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand All @@ -782,6 +787,7 @@ func (s *Suite) TestUploadFile() {
TargetURL: "https://astro.unit.test",
FormFileFieldName: "file",
Headers: map[string]string{},
Description: "This is a test description",
MaxTries: 2,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand Down Expand Up @@ -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?

MaxTries: 2,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand Down
7 changes: 4 additions & 3 deletions software/deploy/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,9 +335,9 @@ func validateIfDagDeployURLCanBeConstructed(deploymentInfo *houston.Deployment)
return nil
}

func getDagDeployURL(deploymentInfo *houston.Deployment, description string) string {
func getDagDeployURL(deploymentInfo *houston.Deployment) string {
c, _ := config.GetCurrentContext()
return fmt.Sprintf("https://deployments.%s/%s/dags/upload?description=%s", c.Domain, deploymentInfo.ReleaseName, description)
return fmt.Sprintf("https://deployments.%s/%s/dags/upload", c.Domain, deploymentInfo.ReleaseName)
}

func DagsOnlyDeploy(houstonClient houston.ClientInterface, appConfig *houston.AppConfig, wsID, deploymentID, dagsParentPath string, dagDeployURL *string, cleanUpFiles bool, description string) error {
Expand Down Expand Up @@ -372,7 +372,7 @@ func DagsOnlyDeploy(houstonClient houston.ClientInterface, appConfig *houston.Ap
if err != nil {
return err
}
uploadURL = getDagDeployURL(deploymentInfo, description)
uploadURL = getDagDeployURL(deploymentInfo)
} else {
uploadURL = *dagDeployURL
}
Expand Down Expand Up @@ -419,6 +419,7 @@ func DagsOnlyDeploy(houstonClient houston.ClientInterface, appConfig *houston.Ap
TargetURL: uploadURL,
FormFileFieldName: "file",
Headers: headers,
Description: description,
MaxTries: 8,
InitialDelayInMS: 1 * 1000,
BackoffFactor: 2,
Expand Down