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

File sync for devfiles #2681

Merged
merged 25 commits into from
Mar 12, 2020
Merged

File sync for devfiles #2681

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c6485ff
Implement devfile syncing on odo push
johnmcollier Mar 4, 2020
98f4c23
Add test for GetRemoteFilesMarkedForDeletion
johnmcollier Mar 4, 2020
0888d70
Properly retrieve sourcePath
johnmcollier Mar 5, 2020
19f0359
Don't push if not needed to
johnmcollier Mar 5, 2020
c017d17
Remove duplicate WaitAndGetPod
johnmcollier Mar 5, 2020
49a620a
Use odo volume constants
johnmcollier Mar 5, 2020
b33cea8
Fix race condition (oops)
johnmcollier Mar 5, 2020
fe7ba17
Fix gosec errors
johnmcollier Mar 5, 2020
a49eadf
Add namespace flag to odo push
johnmcollier Mar 6, 2020
48999ab
Implement integration tests for odo push
johnmcollier Mar 6, 2020
9e593e5
Address review comments on tests
johnmcollier Mar 6, 2020
5f5f59f
Remove duplicated preferences lines
johnmcollier Mar 6, 2020
ec8a692
Merge branch 'master' of github.com:openshift/odo into DevfileSyncing
johnmcollier Mar 10, 2020
03acdfb
Address review comments
johnmcollier Mar 10, 2020
1994334
Fix unit test
johnmcollier Mar 10, 2020
2625db8
Merge branch 'master' of github.com:openshift/odo into DevfileSyncing
johnmcollier Mar 11, 2020
fcd0c07
Address review comments
johnmcollier Mar 11, 2020
3364a1b
Fix rebase
johnmcollier Mar 11, 2020
fbdfc0d
Properly return error if sync fails
johnmcollier Mar 11, 2020
1c2a1fb
Sync to `/projects/<projectName>` when needed
johnmcollier Mar 11, 2020
b6b7e10
Fix unit tests
johnmcollier Mar 11, 2020
c0005c0
Remove debugging line
johnmcollier Mar 11, 2020
a010475
Add more unit tests for functions in pushlocal
johnmcollier Mar 12, 2020
f0fedb9
Add comments
johnmcollier Mar 12, 2020
18ac435
Fix test on Windows
johnmcollier Mar 12, 2020
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
20 changes: 19 additions & 1 deletion pkg/devfile/adapters/kubernetes/component/adapter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ func (a Adapter) createOrUpdateComponent(componentExists bool) (err error) {
func (a Adapter) pushLocal(path string, files []string, delFiles []string, isForcePush bool, globExps []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is doing alot of things, lets try to extract certain portions to make unit testing better.
two suggestions would be be -
1 - https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R254-R264 can be extracted into a function
2 - https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R268-R277 can be extracted into a function

Also if the function has more then 5 args please try to create a struct to pass on data or consider moving it as part of the method's struct if it doesn't change through out the lifecycle of the struct.
e.g. path and ignoredFiles fields might not change throughout the life of Adapter so maybe it can be places as part of the Adapter's struct? - this is just an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

@girishramnani I'm not sure moving those sections into separate functions would make unit testing easier - they both involve exec'ing into an already running container and manipulating the files that exist there, and that can't be mocked with the fake kube client.

Copy link
Contributor

@girishramnani girishramnani Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we should be able to mock it 🤔 ( I think kube/openshift would have a mock clientset for whatever function exec is using ) and it will definitely make unit testing pushLocal easier because when you would write test for pushLocal you would mock the https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R254-R264 and https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R268-R277 functions.

Copy link
Member Author

@johnmcollier johnmcollier Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think what makes it different is that we're interacting with the kube client differently than we do for other resources. We're interacting directly with the rest client, not calling an existing exec function, and I think that doesn't work well with the mock Kube client:

	req := c.KubeClient.CoreV1().RESTClient().
		Post().
		Namespace(c.Namespace).
		Resource("pods").
		Name(podName).
		SubResource("exec").
		VersionedParams(&podExecOptions, scheme.ParameterCodec)

and

	exec, err := remotecommand.NewSPDYExecutor(config, "POST", req.URL())
	if err != nil {
		return errors.Wrapf(err, "unable execute command via SPDY")
	}
	// initialize the transport of the standard shell streams
	err = exec.Stream(remotecommand.StreamOptions{
		Stdin:  stdin,
		Stdout: stdout,
		Stderr: stderr,
		Tty:    tty,
	})

(see https://github.com/openshift/odo/blob/master/pkg/kclient/pods.go#L93)

Copy link
Member Author

@johnmcollier johnmcollier Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As a compromise, rather than putting those sections with the exec into separate functions, what about moving everything before the exec into their own functions (maybe getCommandToCreateFolder and getCommandToDelete)? Then we could still write unit tests.

(Note when moving ExecCMDInContainer over from occlient, I couldn't find any references in the existing odo code that had unit tests for it, or unit tests for functions that called it, so other parts of odo have had this problem too.)

Copy link
Contributor

@girishramnani girishramnani Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

(Note when moving ExecCMDInContainer over from occlient, I couldn't find any references in the existing odo code that had unit tests for it, or unit tests for functions that called it, so other parts of odo have had this problem too.)

and that is the reason that we should unit test these as we have the opportunity to re-write/replace this functionality

Copy link
Contributor

Choose a reason for hiding this comment

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

As a compromise, rather than putting those sections with the exec into separate functions, what about moving everything before the exec into their own functions (maybe getCommandToCreateFolder and getCommandToDelete)? Then we could still write unit tests.

I keep that upto you, I just wanted to break this function down into smaller pieces to make it more unit testable

Copy link
Member Author

@johnmcollier johnmcollier Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@girishramnani I'll go with approach for now (and see if there are any other pieces we can break up). I'll also address your comment regarding the number of parameters.

Since unit tests for the ExecCmdInContainer function will require more work / intevestigation / design, I'll address it as part of #2652 (which is also dealing with unit-testing exec as part of sync)

glog.V(4).Infof("Push: componentName: %s, path: %s, files: %s, delFiles: %s, isForcePush: %+v", a.ComponentName, path, files, delFiles, isForcePush)

syncFolder := kclient.OdoSourceVolumeMount

// Edge case: check to see that the path is NOT empty.
emptyDir, err := util.IsEmpty(path)
if err != nil {
Expand Down Expand Up @@ -246,6 +248,22 @@ func (a Adapter) pushLocal(path string, files []string, delFiles []string, isFor
s := log.Spinner("Syncing files to the component")
defer s.End(false)

// If there's only one project defined in the devfile, sync to `/projects/project-name`, otherwise sync to /projects
projects := a.Devfile.Data.GetProjects()
fmt.Println(len(projects))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an errant println slipped through

Copy link
Member Author

Choose a reason for hiding this comment

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

That was an errant debugging line that slipped through, removed.

if len(projects) == 1 {
syncFolder = filepath.Join(kclient.OdoSourceVolumeMount, projects[0].Name)

// Need to make sure the folder already exists on the component or else sync will fail
reader, writer := io.Pipe()
glog.V(4).Infof("Creating %s on the remote container if it doesn't already exist", syncFolder)
cmdArr := []string{"mkdir", "-p", syncFolder}

err := a.Client.ExecCMDInContainer(pod.Name, containerName, cmdArr, writer, writer, reader, false)
if err != nil {
return err
}
}
// If there were any files deleted locally, delete them remotely too.
if len(delFiles) > 0 {
reader, writer := io.Pipe()
Expand All @@ -270,7 +288,7 @@ func (a Adapter) pushLocal(path string, files []string, delFiles []string, isFor

if isForcePush || len(files) > 0 {
glog.V(4).Infof("Copying files %s to pod", strings.Join(files, " "))
err = sync.CopyFile(&a.Client, path, pod.GetName(), containerName, kclient.OdoSourceVolumeMount, files, globExps)
err = sync.CopyFile(&a.Client, path, pod.GetName(), containerName, syncFolder, files, globExps)
if err != nil {
s.End(false)
return errors.Wrap(err, "unable push files to pod")
Expand Down
5 changes: 5 additions & 0 deletions pkg/devfile/versions/1.0.0/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,8 @@ func (d *Devfile100) GetAliasedComponents() []common.DevfileComponent {
}
return aliasedComponents
}

// GetProjects returns the slice of DevfileProject objects parsed from the Devfile
func (d *Devfile100) GetProjects() []common.DevfileProject {
return d.Projects
}
1 change: 1 addition & 0 deletions pkg/devfile/versions/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ import (
type DevfileData interface {
GetComponents() []common.DevfileComponent
GetAliasedComponents() []common.DevfileComponent
GetProjects() []common.DevfileProject
}