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

Devfile command execution #2735

Merged
merged 15 commits into from
Apr 3, 2020

Conversation

maysunfaisal
Copy link
Contributor

@maysunfaisal maysunfaisal commented Mar 19, 2020

Signed-off-by: Maysun J Faisal maysun.j.faisal@ibm.com

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR:

  • executes devBuild and devRun commands from the devfile
  • bootstraps supervisord to the devfile run component container from an init container
  • makes supervisord the entrypoint of the devfile run component container if no entrypoint is mentioned in the devfile
  • executes the devRun command with supervisord
  • introduces two new push flags build-command & run-command which can be used instead of devBuild and devRun
  • unit tests

Which issue(s) this PR fixes:

Fixes #2654

How to test changes / Special notes to the reviewer:

Until redhat-developer/odo-init-image#52 is merged, pls set ODO_BOOTSTRAPPER_IMAGE=jeevandroid/odo-init-image for testing with the redhat-developer/odo-init-image#52 changes.

odo push
odo push --show-log
odo push --build-command="some build command" --run-command="some run command"

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Mar 19, 2020
Copy link
Contributor

@rajivnathan rajivnathan left a comment

Choose a reason for hiding this comment

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

Initial feedback

"github.com/golang/glog"

"github.com/openshift/odo/pkg/devfile/versions"
"github.com/openshift/odo/pkg/devfile/versions/common"
)

const (
DefaultDevfileBuildCommand = "devBuild"
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 probably make these lowercase so that we can easily convert the name from the devfile to lowercase and compare directly. Also we can group these together as a type.

eg.

type PredefinedDevfileCommands string

const (
	DefaultDevfileBuildCommand PredefinedDevfileCommands = "devbuild"
	DefaultDevfileRunCommand PredefinedDevfileCommands = "devrun"
	DefaultDevfileDebugCommand PredefinedDevfileCommands = "devdebug"
	DefaultDevfileTestCommand PredefinedDevfileCommands = "devtest"
)

@@ -2,7 +2,7 @@ package common

// ComponentAdapter defines the functions that platform-specific adapters must implement
type ComponentAdapter interface {
Push(path string, ignoredFiles []string, forceBuild bool, globExps []string) error
Push(path string, ignoredFiles []string, forceBuild bool, show bool, globExps []string, devfileBuildCmd, devfileRunCmd 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.

Should we create a struct to group all these together?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, they should be in a struct. And FWIW, my odo watch PR groups these into a struct as well 😉

Copy link
Member

Choose a reason for hiding this comment

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

Should we create a struct to group all these together?

That would be great. 👍

for i, container := range containers {
for _, runCommandComponent := range runCommandComponents {
// Check if the container belongs to a run command component
if reflect.DeepEqual(container.Name, runCommandComponent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use reflect.DeepEqual here? Seems like we can just use the == comparator?

// Always mount the supervisord volume in the run component container
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: kclient.GetSupervisordVolumeName(),
MountPath: "/opt/odo/",
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 use a constant for the MountPath value

@@ -19,6 +19,14 @@ const (
Please ensure you have an active kubernetes context to your cluster.
Consult your Kubernetes distribution's documentation for more details
`
// The init container name for supervisord
supervisordInitContainerName = "copy-supervisord"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move these to the supervisord.go file?

@@ -20,3 +31,160 @@ func GetSupportedComponents(data versions.DevfileData) []common.DevfileComponent
}
return components
}

// GetCommand iterates through the devfile commands and returns the associated devfile command
func GetCommand(data versions.DevfileData, commandName string) (command common.DevfileCommand) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this function be in the devfile parser instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

devfile parser has GetCommands() which returns all the commands from the devfile and is defined in the interface like the other devfile helper functions.

utils.GetCommand() takes a command name as input and returns one command from the devfile parser's GetCommands() output

@@ -35,17 +37,30 @@ func New(adapterContext common.AdapterContext, client kclient.Client) Adapter {
type Adapter struct {
Client kclient.Client
common.AdapterContext
pod *corev1.Pod
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need pod in the struct? Should it or a just the necessary values from the pod just be passed along as needed?

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't store the pod in the struct. And we also would need to update it every time the pod got recreated

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 21, 2020
@@ -35,17 +37,30 @@ func New(adapterContext common.AdapterContext, client kclient.Client) Adapter {
type Adapter struct {
Client kclient.Client
common.AdapterContext
pod *corev1.Pod
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't store the pod in the struct. And we also would need to update it every time the pod got recreated


// If neither is validated, iterate the devfile commands to see for a list of supported commands
for _, supportedCommand := range devfileSupportedCommands {
if reflect.DeepEqual(supportedCommand.Name, DefaultDevfileBuildCommand) && !validateBuildCommand {
Copy link
Member

Choose a reason for hiding this comment

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

We can safely compare strings directly in Go, so we shouldn't need the refelct.DeepEqual for comparing the string names

@@ -1,5 +1,5 @@
package adapters

type PlatformAdapter interface {
Push(path string, ignoredFiles []string, forceBuild bool, globExps []string) error
Push(path string, ignoredFiles []string, forceBuild bool, show bool, globExps []string, devfileBuildCmd, devfileRunCmd string) error
Copy link
Member

Choose a reason for hiding this comment

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

As Rajiv mentioned higher up, this function and the other push function should be modified to accept a struct.

See my odo watch PR for how I did it: https://github.com/openshift/odo/blob/a727c2d31fe402a73f9beab046d72366a11123f7/pkg/devfile/adapters/common/types.go#L27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, adopted PushParameters from the Watch PR, will rebase on other changes when it is merged

fallthrough
case DefaultDevfileRunCommand:
fallthrough
case DefaultDevfileDebugCommand:
Copy link
Member

Choose a reason for hiding this comment

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

I see we're adding in cases for DefaultDevfileDebugCommand and DefaultDevfileTestCommand. Should the Push function be updated to take those in too? Right now it only accepts `[]string, devfileBuildCmd, devfileRunCmd string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will be having a new command for test and debug like odo execute. They will not be executed through odo push. I am removing the debug and test command name constants to avoid confusion. They can be re-added later on

// Validate the devfile build and run commands
pushDevfileCommands, err := common.ValidateAndGetPushDevfileCommands(a.Devfile.Data, a.devfileBuildCmd, a.devfileRunCmd)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Can you wrap this error with a message? Maybe something like "Push failed due to validate error"?

if err != nil {
return errors.Wrapf(err, "error while waiting for pod %s", podSelector)
}
a.pod = pod
Copy link
Member

Choose a reason for hiding this comment

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

Since WaitAndGetPod doesn't wait for a new pod to rollout and for the old one to go away, there could be a race-condition here where the old pod is returned (before it was moved to the terminating state) instead of the new one.

This may be more of a matter of waiting for #2731 to get merged and to use the "Wait for Rollout" function instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

#2731 adds the wait for rollout to this exact function so we shouldn't need to add any additional waits on this function.

@@ -292,6 +321,187 @@ func (a Adapter) pushLocal(path string, files []string, delFiles []string, isFor
return nil
}

// Push syncs source code from the user's disk to the component
func (a Adapter) execDevfile(pushDevfileCommands []versionsCommon.DevfileCommand, componentExists, show bool) error {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering...

We'll need the exec code to be pretty consistent between local (Docker) and Kube. I'm wondering if we should move this out to a new package, like we did for sync (let's call it exec). Then define an interface (let's call it ExecClient) that has one function: ExecCmdInContainer, and both the kclient and lclient would implement it.

It's up to you, but moving this out to its own package now, makes our lives easier for Docker support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, let me do it like the way you did for sync, to be consistent


for _, action := range command.Actions {
// Change to the workdir and execute the command
cmdArr := []string{"/bin/sh", "-c", "cd " + *action.Workdir + " && " + *action.Command}
Copy link
Member

Choose a reason for hiding this comment

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

workdir is an optional field in devfile
If it is not specified the command should be executed in default image directory (no cding just executing the command)

Copy link
Member

Choose a reason for hiding this comment

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

also action.Command is a pointer, so it can be nil, so you should check it before using it, otherwise, it can end with panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, i have already done that in my new commit which i didnt push

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i shall make the changes for workdir 👍

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 25, 2020
@maysunfaisal maysunfaisal marked this pull request as ready for review March 25, 2020 23:34
@maysunfaisal
Copy link
Contributor Author

maysunfaisal commented Mar 25, 2020

@rajivnathan @johnmcollier @kadel PTAL at your convenience. I have addressed the review comments and will be working on the tests for the PR. Thx!

supervisordInitContainerName = "copy-supervisord"

// The default image for odo init bootstrapper container
defaultBootstrapperImage = "jeevandroid/odo-init-image"
Copy link
Member

Choose a reason for hiding this comment

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

Most of the constants here are already defined in occlient.go (https://github.com/openshift/odo/blob/a8a5eca39b80abf693bbe1e5e98880f9f31bf34a/pkg/occlient/occlient.go#L109)

We should probably abstract it away to one common place and use the same const everywhere. Or just export it occlient and use it here for now, but having it defined in two places will get messy and prone to bugs :-/

Copy link
Member

Choose a reason for hiding this comment

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

Or even better you could modify occlient to use const from here, as there is a bigger chance that this will become long term place to store this.

supervisordInitContainerName = "copy-supervisord"

// The default image for odo init bootstrapper container
defaultBootstrapperImage = "jeevandroid/odo-init-image"
Copy link
Member

Choose a reason for hiding this comment

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

Most of the constants here are already defined in occlient.go (https://github.com/openshift/odo/blob/a8a5eca39b80abf693bbe1e5e98880f9f31bf34a/pkg/occlient/occlient.go#L109)

We should probably abstract it away to one common place and use the same const everywhere. Or just export it occlient and use it here for now, but having it defined in two places will get messy and prone to bugs :-/

podTemplateSpec.Spec.InitContainers = append(podTemplateSpec.Spec.InitContainers,
corev1.Container{
Name: supervisordInitContainerName,
Image: defaultBootstrapperImage,
Copy link
Member

Choose a reason for hiding this comment

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

The image can be overwritten with ODO_BOOTSTRAPPER_IMAGE env variable. This is used in tests and on clusters with restricted network access (image is mirrored to an internal registry)

You should use something like this

https://github.com/openshift/odo/blob/a8a5eca39b80abf693bbe1e5e98880f9f31bf34a/pkg/occlient/occlient.go#L224

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx for reminding me about this, i saw this when going through the odo non experimental path but forgot as i was doing this branch..

@@ -0,0 +1,71 @@
package kclient
Copy link
Member

Choose a reason for hiding this comment

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

A lot of the things in this file aren't specific to Kube (other than AddBootstrapSupervisordInitContainer), could we move this into somewhere common so that it could be used for Docker and Kube odo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, i will see where i can move them

@kadel
Copy link
Member

kadel commented Mar 27, 2020

This is great start @maysunfaisal

I did a couple of tests and it works as I would expect!
It would be just nice to have source code a little bit more integrated with the rest of the code base.

@maysunfaisal
Copy link
Contributor Author

maysunfaisal commented Mar 27, 2020

This is great start @maysunfaisal

I did a couple of tests and it works as I would expect!
It would be just nice to have source code a little bit more integrated with the rest of the code base.

@kadel thx, other than the constants, anything you have in mind?

EDIT:

I have also moved out the exec code into its own package exec, since i think a lot of code will be using it. What do you think about the occlient using it? The new exec is a tad bit different but copied from the exec code of assemble-and-restart; we use the reader pipe to output to stdout when --show or log debug is provided.

Copy link
Contributor

@rajivnathan rajivnathan left a comment

Choose a reason for hiding this comment

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

I've been using this with che devfiles and things are working really well! Have some further questions/comments.

@@ -20,3 +30,158 @@ func GetSupportedComponents(data versions.DevfileData) []common.DevfileComponent
}
return components
}

// GetCommand iterates through the devfile commands and returns the associated devfile command
func GetCommand(data versions.DevfileData, commandName string) (supportedCommand common.DevfileCommand) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these functions are specific to commands. To prevent utils.go becoming very large should we maybe move these to a separate file called commands.go?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can separate things out afterwards since moving it now will complicated handling merge conflicts

return supportedCommand
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add an info level log here so that it's clear the command was not found?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added commit 14d81a5 to address this

supportedCommandActions := getSupportedCommandActions(command)

// if there is a supported command action of type exec, save it
if len(supportedCommandActions) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be an error if a command is found but there are no supportedCommandActions? I'm thinking it is because there's effectively nothing for odo to execute and the user may be left wondering why nothing is happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added commit 14d81a5 to address this

func ValidateAndGetPushDevfileCommands(data versions.DevfileData, devfileBuildCmd, devfileRunCmd string) ([]common.DevfileCommand, error) {
var pushDevfileCommands []common.DevfileCommand
// var emptyCommand common.DevfileCommand
validateBuildCommand, validateRunCommand := false, false
Copy link
Contributor

Choose a reason for hiding this comment

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

I initially thought these variables were being used to decide whether to validate or not but it's actually the validation result. Can you please rename validateBuildCommand to isBuildCommandValid
and
validateRunCommand to isRunCommandValid to make it a bit more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added commit 5378ed9 to address this

}

// IsCommandPresent checks if the given command is empty or not
func IsCommandPresent(command common.DevfileCommand) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get rid of this function if GetCommand() returns an error when there are no supportedCommandActions

Copy link
Contributor

Choose a reason for hiding this comment

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

I added commit 14d81a5 to address this

validateBuildCommand, validateRunCommand := false, false

buildCommand := GetBuildCommand(data, devfileBuildCmd)
if devfileBuildCmd == "" && !IsCommandPresent(buildCommand) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do the changes mentioned for GetCommand() then this logic can be simplified to just check whether GetBuildCommand() returns an error and then IsCommandValid(). Same for the run command.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added commit 14d81a5 to address this

// validateAction validates the given action
// 1. action has to be of type exec 2. component should be present
// 3. command should be present
func validateAction(action common.DevfileCommandAction) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than returning a boolean, I'd suggest to return an error with a clear message for each validation problem. That way when surfacing the error to the user they know what exactly is wrong with their devfile. Instead of checking for true/false we can check err !=nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added commit 14d81a5 to address this

}

if !validateBuildCommand || !validateRunCommand {
return []common.DevfileCommand{}, fmt.Errorf("devfile command validation failed, validateBuildCommand: %v validateRunCommand: %v", validateBuildCommand, validateRunCommand)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is how the error currently looks for the user:

✗  Failed to start component with name openlibertydevfile.
Error: Failed to create the component: failed to validate devfile build and run commands: devfile command validation failed, validateBuildCommand: true validateRunCommand: false

With the changes to validateAction() we can just print each of the error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added commit 5378ed9 to address this

namespace string
}

// DevfileCommand encapsulates the build and run command
// which can be explicitly provided during odo push
type DevfileCommand struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? Don't believe it's used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, i forgot to remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed in commit 14d81a5

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 27, 2020
@maysunfaisal maysunfaisal changed the title {WIP} Devfile command execution Devfile command execution Mar 31, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Mar 31, 2020
@kadel kadel mentioned this pull request Apr 1, 2020
Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

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

Tried it out, works well!! some nits


"github.com/openshift/odo/pkg/devfile/versions"
"github.com/openshift/odo/pkg/devfile/versions/common"
)

type PredefinedDevfileCommands string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this also can be defined in pkg/devfile/adapters/common/types.go and DevfileBuildCmd in PushParameters could be of type PredefinedDevfileCommands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this after your suggestion.

So the PushParameters here are the options from the odo push command where as PredefinedDevfileCommands is a type for command names which are pre-defined and supported by odo experimental for devfile. These predefined commands are going to be devbuild, devrun, devdebug & devtest... For now only devbuild and devrun have been defined and implemented. I expect others to be defined and implemented in the upcoming sprints.

So,

  • commands names devbuild, devrun are of type PredefinedDevfileCommands from the devfile
    odo push will use PredefinedDevfileCommands command devbuild for build and PredefinedDevfileCommands command devrun for run

  • command names randomcommand123, randomcommand456 are of type string for PushParameters from odo push
    odo push --build-command="randomcommand123" will use command randomcommand123 for build and PredefinedDevfileCommands command devrun for run
    odo push --build-command="randomcommand123" --run-command="randomcommand456" will use command randomcommand123 for build and command randomcommand456 for run


// type must be exec
if *action.Type != common.DevfileCommandTypeExec {
return fmt.Errorf("Actions must be of type \"exec\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

could we print here action name and other places below as there could be multiple actions in devfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actions dont have a name in a devfile

commands:
  -
    name: devRun
    actions:
      -
        type: exec
        component: runtime
        command: "/artifacts/bin/start-server.sh"
        workdir: /

But we are printing what action number is an error when we call this function to validate

for i, action := range command.Actions {
		// Check if the command action is of type exec
		err := validateAction(data, action)
		if err == nil {
			glog.V(3).Infof("Action %d maps to component %v", i+1, *action.Component)
			supportedCommandActions = append(supportedCommandActions, action)
		} else {
			problemMsg += fmt.Sprintf("\nProblem with command \"%v\" action #%d: %v", command.Name, i+1, err)
		}
	}

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 1, 2020
maysunfaisal and others added 13 commits April 2, 2020 08:33
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Rajiv Senthilnathan <rajivsen@ca.ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

Overall changes look good and work great for me, so approving this from my end

/lgtm Great work Maysun, really happy to see this get in.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 2, 2020
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 3, 2020
@kadel
Copy link
Member

kadel commented Apr 3, 2020

/test ci/prow/unit

@kadel
Copy link
Member

kadel commented Apr 3, 2020

/lgtm
^ was already lgtmed by @johnmcollier

/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 3, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 3, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit fd8416c into redhat-developer:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execute commands during Devfile push
8 participants