Devfile Adapter Cleanup/Refactor

[maysunfaisal]

/kind user-story

User Story

Some cleanups to be tracked, that were raised during #2922

Acceptance Criteria

• Should clean up build command. Instead of appending, they should be separated out by commas https://github.com/openshift/odo/blob/master/pkg/exec/devfile.go#L20 (N/A - reason Devfile Adapter Cleanup/Refactor #3015 (comment))
• Refactor docker/utils/utils.go StartBootstrapSupervisordInitContainer() since it is very similar to docker/component/utils.go startComponent() Devfile Adapter Cleanup/Refactor #3015 (comment)
• sync/adapter.go SyncFiles() has a lot of variables, see if they can be cleaned up further nicely with a struct Devfile Adapter Cleanup/Refactor #3015 (comment)
• Refactor the project vol creation code, during component update if the project vol is not present, create it and sync the project Fix Docker Supervisord Vol Initialization #3129
• Check if kube and docker adapter's execDevfile() can be refactored more cleanly Devfile Adapter Cleanup/Refactor #3015 (comment)
• Put up a heading during initializating of supervisord if --show-log is absent, it takes some time to pull the docker image down, and the push output does not show anything at the time [Docker] User left waiting with no spinner while supervisord image is pulling #3033
• During component update on kube, show the url link when mentioning it, currently just says url already exist (N/A - reason Devfile Adapter Cleanup/Refactor #3015 (comment))
• Update Supervisord volume name to be unique for each volume Update Docker Supervisord Initialization Spinner and Supervisord Vol Name #3120

Links

• Feature Request:
• Related issue:

/kind user-story

[maysunfaisal]

/area devfile

[girishramnani]

I would suggest we do the cleanup once we have devfilev2

[girishramnani]

we need to break this issue into two parts, devfile dependent and independent tasks.

[maysunfaisal]

I went through the above checklist, and looks like none of the items are hard devfile dependent. The only possible items that MAYBE affected are:

• Refactor docker/utils/utils.go StartBootstrapSupervisordInitContainer() since it is very similar to docker/component/utils.go startComponent()
• Check if kube and docker adapter's execDevfile() can be refactored more cleanly

Would need to go through the individual devfile v2 feature request to see if they would be truly affected!

[maysunfaisal]

Updates:

• not changing the url code from the checklist because it now says URL testspr5 successfully deleted
✓ URL testspr5: http://testspr5-faisal.xxxx.com created on successive push

• Cannot clean up build command because the docker/kube client API expects a command to be executed. For example,
  our run command is command: []string{adaptersCommon.SupervisordBinaryPath, adaptersCommon.SupervisordCtlSubCommand, "start", string(adaptersCommon.DefaultDevfileRunCommand)} and this is fine because we're executing the supervisord binary adaptersCommon.SupervisordBinaryPath along with its options.

Similarly the build command in master branch is, cmdArr = []string{adaptersCommon.ShellExecutable, "-c", "cd " + exec.WorkingDir + " && " + exec.CommandLine} and there is no way to avoid appending like cmdArr = []string{"cd ", exec.WorkingDir, "&&", exec.CommandLine}. /bin/sh executable adaptersCommon.ShellExecutable is the command and the rest of them are the options.

To break it down even further,
I can do cmdArr = []string{"date"} as this is a valid command
but i cant do cmdArr = []string{"date", ">>", "/tmp/may.txt"} or cmdArr = []string{"date >> /tmp/may.txt"}, because in the first case ">>", "/tmp/may.txt" are not date options; and in the second case "date >> /tmp/may.txt" is not a recognized command.

[maysunfaisal]

Update:
Will skip on checklist Refactor docker/utils/utils.go StartBootstrapSupervisordInitContainer() since it is very similar to docker/component/utils.go startComponent() because looking at it startComponent() does so much than bootstrap func() like update containerConfig with ports and update hostConfig with ENV for commands.

I can see it being refactored with a func() that takes hostConfig/containerConfig and starts the container. But since this is docker, I am going to skip on it until reqd.

[maysunfaisal]

Update:
Checking off sync/adapter.go SyncFiles() has a lot of variables, see if they can be cleaned up further nicely with a struct

SyncFiles(syncParameters common.SyncParameters) is already cleaned up with structs:

type SyncParameters struct {
	PushParams      PushParameters
	CompInfo        ComponentInfo
	PodChanged      bool
	ComponentExists bool
}

type PushParameters struct {
	Path              string                  // Path refers to the parent folder containing the source code to push up to a component
	WatchFiles        []string                // Optional: WatchFiles is the list of changed files detected by odo watch. If empty or nil, odo will check .odo/odo-file-index.json to determine changed files
	WatchDeletedFiles []string                // Optional: WatchDeletedFiles is the list of deleted files detected by odo watch. If empty or nil, odo will check .odo/odo-file-index.json to determine deleted files
	IgnoredFiles      []string                // IgnoredFiles is the list of files to not push up to a component
	ForceBuild        bool                    // ForceBuild determines whether or not to push all of the files up to a component or just files that have changed, added or removed.
	Show              bool                    // Show tells whether the devfile command output should be shown on stdout
	DevfileInitCmd    string                  // DevfileInitCmd takes the init command through the command line and overwrites devfile init command
	DevfileBuildCmd   string                  // DevfileBuildCmd takes the build command through the command line and overwrites devfile build command
	DevfileRunCmd     string                  // DevfileRunCmd takes the run command through the command line and overwrites devfile run command
	DevfileDebugCmd   string                  // DevfileDebugCmd takes the debug command through the command line and overwrites the devfile debug command
	EnvSpecificInfo   envinfo.EnvSpecificInfo // EnvSpecificInfo contains infomation of env.yaml file
	Debug             bool                    // Runs the component in debug mode
	DebugPort         int                     // Port used for remote debugging
}

Also checked that pushParameters := syncParameters.PushParams is used instead of the long qualified names to access push param struct properties

[maysunfaisal]

Update:
execDevfile() could have been refactored between kube and docker adapter. But it looks like kube and docker implementations have diverged. Kube has debug and restart capability while docker does not.

I also noticed that we have ExecuteDevfileRunActionWithoutRestart(), ExecuteDevfileDebugActionWithoutRestart which is 95% the same as their non-restart counter parts. Not sure why these functions were written instead of modifying the existing ones. I am opening an issue to clean this up - #3518

and closing this