bug 1470374 - oc new-app behaviour

[mjudeikis]

oc new-app

oc import-image openshift/ruby-20-centos7:latest --confirm -n openshift

Syntax 1. Case 1. Git: True. URL: Remote.

RESULT: All components created. Source build type.
EXPECTATION: OK
oc new-app openshift/ruby-20-centos7:latest~https://github.com/openshift/ruby-hello-world.git

Syntax 1. Case 2. Git: False. URL: Remote.

RESULT: All components created. Source build type.
EXPECTATION: OK
oc new-app openshift/ruby-20-centos7:latest~https://github.com/openshift/ruby-hello-world.git

-bash-4.4# oc new-app openshift/ruby-20-centos7:latest~https://github.com/openshift/ruby-hello-world.git
warning: Cannot find git. Ensure that it is installed and in your path. Git is required to work with git repositories.
....

Syntax 1. Case 3. GIT: False. URL: HostPath.

RESULT: All components created. BINARY build type.
EXPECTATION: ok
oc new-app openshift/ruby-20-centos7:latest~/root/app-examples/ruby

warning: Cannot find git. Ensure that it is installed and in your path. Git is required to work with git repositories.
...

Syntax 1. Case 4. Git: True. URL: HostDir.

RESULT: (?TBC?)
EXPECTATION: Check if hostDir is valid git - Source. Else - Binary.
oc new-app openshift/ruby-20-centos7:latest~/root/app-examples/ruby

Syntax 2. Case 1. GIT: True. URL: Remote.

RESULT: All components created. Source build type.
EXPECTATION: OK
oc new-app -i ruby-20-centos7:latest https://github.com/openshift/ruby-hello-world.git

Syntax 2. Case 2. GIT: False. URL: Remote

RESULT: No components created. We cant investigate remote repo.
EXPECTATION: Create all components same as Syntax 1. Case 2
oc new-app -i ruby-20-centos7:latest https://github.com/openshift/ruby-hello-world.git

warning: Cannot find git. Ensure that it is installed and in your path. Git is required to work with git repositories.
error: git binary not available

Syntax 2. Case 3. GIT: False. URL: HostPath.

RESULT: All components created. BINARY build type.
EXPECTATION: ok
oc new-app -i ruby-20-centos7:latest /root/app-examples/ruby

Syntax 2. Case 4. GIT: True. URL: HostPath.

RESULT: (?TBC?)
EXPECTATION: Check if hostDir is valid git - Source. Else - Binary.
oc new-app -i ruby-20-centos7:latest /root/app-examples/ruby

Sytnax 3. Case 1. GIT: True. URL: Remote.

RESULT: All components created. Source build type.
EXPECTATION: ok
oc new-app -i ruby-20-centos7:latest --code https://github.com/openshift/ruby-hello-world.git

Sytnax 3. Case 2. GIT: False. URL: Remote.

RESULT: All components created. Source build type.
EXPECTATION: ok
oc new-app -i ruby-20-centos7:latest --code https://github.com/openshift/ruby-hello-world.git

Sytnax 3. Case 3. GIT: False. URL: HostPath.

RESULT: All components created. Binary build type.
EXPECTATION: ok
oc new-app -i ruby-20-centos7:latest --code /root/app-examples/ruby

Sytnax 3. Case 4. GIT: True. URL: HostPath.

RESULT: (?TBC?)
EXPECTATION: If hostDir - git repo - Source build. Else Binary build.
oc new-app -i ruby-20-centos7:latest --code /root/app-examples/ruby

TODO:

• add tests to cover this case

https://bugzilla.redhat.com/show_bug.cgi?id=1470374

[mjudeikis]

test adding is pending as I can see strange behavior in failing tests due oc delete all -l label=label fails to delete some components. checking if this is a different bug or my environment.

[jim-minter]

Hi @mjudeikis I think this PR is on the wrong track. Currently on master:

1. oc new-app -i openshift/ruby-20-centos7:latest https://github.com/openshift/ruby-hello-world fails. Expected. -i specifies an imagestream, you specified a docker image.
2. oc new-app --image-stream=openshift/ruby-20-centos7:latest --code https://github.com/openshift/ruby-hello-world fails. Expected. Identical case to 1).
3. oc new-app openshift/ruby-20-centos7:latest~https://github.com/openshift/ruby-hello-world succeeds. Expected. It's not specified what "openshift/ruby-20-centos7:latest" is, so new-app tries multiple resolvers.

Adding the docker image resolver to the -i | --image-stream option doesn't make sense to me.

To resolve bz1470374 I'd expect:

1. investigation of why oc new-app -i imagestream --code url thinks it needs to call git and oc new-app imagestream~url doesn't; then see if the former case can be reduced to the latter one
2. (for bonus points) investigation of whether the horrible error messages when running oc new-app -i imagestream url when git isn't installed can be simplified without incurring massive code changes

[mjudeikis]

Update:

read the same comment with a fresh head.
I think for current case 1-2 we just need to update error messages to give a better message on image vs image-stream.

to resolve - checking.

[mjudeikis]

investigation of why oc new-app -i imagestream --code url thinks it needs to call git and oc new-app imagestream~url doesn't; then see if the former case can be reduced to the latter one

Both use-cases are calling git. even if you pass git url OR local cloned repo, it calls remote git. Question:
if I do: oc new-app ruby-20-centos7~/root/app-examples/ruby-hello-world should build point to remote git or should it be binary build?

$ oc new-app openshift/ruby-20-centos7:latest~/home/user/code/my-ruby-app
https://docs.openshift.com/container-platform/3.6/dev_guide/application_lifecycle/new_app.html#dev-guide-new-app

(for bonus points) investigation of whether the horrible error messages when running oc new-app -i imagestream url when git isn't installed can be simplified without incurring massive code changes

cmd/newapp.go:

result, err := config.Run()
	if err := handleError(err, o.BaseName, o.CommandName, o.CommandPath, config, transformRunError); err != nil {
		return err
	}

to:

	result, err := config.Run()
	if err := handleError(err, o.BaseName, o.CommandName, o.CommandPath, config, transformRunError); err != nil {
		if strings.Contains(err.Error(), git.ErrGitNotAvailable.Error()) {
			//failure was due missing git binary. We flush all error stack and give one nice error instead. 
			return git.ErrGitNotAvailable
		}
		return err
	}

-bash-4.4# oc new-app -i ruby-20-centos7 https://github.com/openshift/ruby-hello-world
warning: Cannot find git. Ensure that it is installed and in your path. Git is required to work with git repositories.
error: git binary not available

[bparees]

Both use-cases are calling git. even if you pass git url OR local cloned repo, it calls remote git.

It shouldn't be calling remote git if you have told it what image to build with. That's the crux of the bug.

And @jim-minter indicated that indeed it is not calling git in that situation (See his scenario 1):
https://bugzilla.redhat.com/show_bug.cgi?id=1470374#c6

If you provide an explicit builder image, we should not need to clone your repo because we don't need to attempt to identify what the application type is. (We might attempt to clone it anyway just to see if credentials are required to access your repo, however that should result in a warning, not a failure, if we are unsuccessful).

Question:
if I do: oc new-app ruby-20-centos7~/root/app-examples/ruby-hello-world should build point to remote git or should it be binary build?

remote git, assuming the dir has a valid git remote. We should always prefer creating a buildconfig with a git reference over creating a binary build.

[mjudeikis]

If you provide an explicit builder image, we should not need to clone your repo because we don't need to attempt to identify what the application type is. (We might attempt to clone it anyway just to see if credentials are required to access your repo, however, that should result in a warning, not a failure, if we are unsuccessful).

But if we say "check for auth is important" we cant eliminate usage of git command it this case as we still need git to do that... Looks like "its a feature, not a bug" case... I would like to keep "auth checking", but as discussed with @jim-minter, we might want to reconfirm what kind of behavior we expect from these commands. Question is - is it worth it..

[bparees]

But if we say "check for auth is important" we cant eliminate usage of git command it this case as we still need git to do that

inability to check for auth should be a warning. in ability to git clone your repo is an error.

thus we should only attempt to git clone your repo if we need to. and the root of the bug is that we are git cloning your repo in cases where we do not need to (because we already know what builder image will be used with it).

[mjudeikis]

UPDATED:
As per comments added : EXPECTATION field.

Updated 2: moved to PR description

[mjudeikis]

@bparees ping

[bparees]

@mjudeikis i think my request for tests still needs to be addressed, otherwise lgtm.

[bparees]

/lgtm

thank you for your perseverance @mjudeikis

[mjudeikis]

@bparees you fast :D too fast (one more test added :/ sorry :D )

[bparees]

/lgtm

[mjudeikis]

/test extended_conformance_gce

[mjudeikis]

/lgtm

[openshift-ci-robot]

@mjudeikis: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[bparees]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, mjudeikis

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

Needs approval from an approver in each of these OWNERS Files:

• pkg/generate/OWNERS [bparees]
• pkg/oc/OWNERS [bparees]
• test/cmd/OWNERS [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[openshift-bot]

/retest

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

[openshift-bot]

/retest

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

[openshift-bot]

/retest

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

[openshift-merge-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[openshift-merge-robot]

Automatic merge from submit-queue (batch tested with PRs 17539, 17457).

[openshift-ci-robot]

@mjudeikis: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install	4113dc5	link	/test extended_conformance_install

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.