- Notifications
You must be signed in to change notification settings - Fork928
fix: clean template destination path forpull#12559
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
5306e069e037cd00d5e61d15b5e380addee9d035e35bf6dd60fc2275b4114beFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -230,118 +230,116 @@ func TestTemplatePull_LatestStdout(t *testing.T) { | ||
| // ToDir tests that 'templates pull' pulls down the active template | ||
| // and writes it to the correct directory. | ||
| // | ||
| // nolint: paralleltest // The subtests cannot be run in parallel; see the inner loop. | ||
| func TestTemplatePull_ToDir(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. How do these tests actually work with chdir removed? The files will be created inside the repo since we're not in the tmpdir, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. It pulls into the inv,root:=clitest.New(t,"templates","pull",template.Name,tc.givenPath) AFAICS it's functionally equivalent Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. If the given path is relative, won't it be relative to the Go process, i.e. the test which is running in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. You're correct! I added these two lines to account for that in the last commit: newWD:=t.TempDir()require.NoError(t,os.Chdir(newWD)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. You also need to restore the wd after the test has ended, otherwise it could break other tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Oooh good point! | ||
| tests := []struct { | ||
| name string | ||
| destPath string | ||
| useDefaultDest bool | ||
| }{ | ||
| { | ||
| name: "absolute path works", | ||
| useDefaultDest: true, | ||
| }, | ||
| { | ||
| name: "relative path to specific dir is sanitized", | ||
| destPath: "./pulltmp", | ||
| }, | ||
| { | ||
| name: "relative path to current dir is sanitized", | ||
| destPath: ".", | ||
| }, | ||
| { | ||
| name: "directory traversal is acceptable", | ||
| destPath: "../mytmpl", | ||
| }, | ||
| { | ||
| name: "empty path falls back to using template name", | ||
| destPath: "", | ||
| }, | ||
| } | ||
| // nolint: paralleltest // These tests change the current working dir, and is therefore unsuitable for parallelisation. | ||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| dir := t.TempDir() | ||
| cwd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, os.Chdir(cwd)) | ||
| }) | ||
| // Change working directory so that relative path tests don't affect the original working directory. | ||
| newWd := filepath.Join(dir, "new-cwd") | ||
| require.NoError(t, os.MkdirAll(newWd, 0o750)) | ||
| require.NoError(t, os.Chdir(newWd)) | ||
| expectedDest := filepath.Join(dir, "expected") | ||
| actualDest := tc.destPath | ||
| if tc.useDefaultDest { | ||
| actualDest = filepath.Join(dir, "actual") | ||
| } | ||
| client := coderdtest.New(t, &coderdtest.Options{ | ||
| IncludeProvisionerDaemon: true, | ||
| }) | ||
| owner := coderdtest.CreateFirstUser(t, client) | ||
| templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) | ||
| // Create an initial template bundle. | ||
| source1 := genTemplateVersionSource() | ||
| // Create an updated template bundle. This will be used to ensure | ||
| // that templates are correctly returned in order from latest to oldest. | ||
| source2 := genTemplateVersionSource() | ||
| expected, err := echo.Tar(source2) | ||
| require.NoError(t, err) | ||
| version1 := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, source1) | ||
| _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version1.ID) | ||
| template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version1.ID) | ||
| // Update the template version so that we can assert that templates | ||
| // are being sorted correctly. | ||
| updatedVersion := coderdtest.UpdateTemplateVersion(t, client, owner.OrganizationID, source2, template.ID) | ||
| _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, updatedVersion.ID) | ||
| coderdtest.UpdateActiveTemplateVersion(t, client, template.ID, updatedVersion.ID) | ||
| ctx := context.Background() | ||
| err = extract.Tar(ctx, bytes.NewReader(expected), expectedDest, nil) | ||
| require.NoError(t, err) | ||
| ents, _ := os.ReadDir(actualDest) | ||
| if len(ents) > 0 { | ||
| t.Logf("%s is not empty", actualDest) | ||
| t.FailNow() | ||
| } | ||
| inv, root := clitest.New(t, "templates", "pull", template.Name, actualDest) | ||
| clitest.SetupConfig(t, templateAdmin, root) | ||
| ptytest.New(t).Attach(inv) | ||
| require.NoError(t, inv.Run()) | ||
| // Validate behaviour of choosing template name in the absence of an output path argument. | ||
| destPath := actualDest | ||
| if destPath == "" { | ||
| destPath = template.Name | ||
| } | ||
| require.Equal(t, | ||
| dirSum(t, expectedDest), | ||
| dirSum(t, destPath), | ||
| ) | ||
| }) | ||
| } | ||
| } | ||
| // FolderConflict tests that 'templates pull' fails when a folder with has | ||