Skip to content

bug 1470374 - oc new-app behaviour #17457

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

Merged
merged 1 commit into from
Jan 12, 2018
Merged

bug 1470374 - oc new-app behaviour #17457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions pkg/generate/app/sourcelookup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ func (r *SourceRepository) DetectAuth() error {
_, _, err = gitRepo.TimedListRemote(10*time.Second, url.StringNoFragment(), "--heads")
if err != nil {
r.requiresAuth = true
fmt.Print("warning: Cannot check if git requires authentication.\n")
}
return nil
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/oc/generate/app/cmd/newapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,9 @@ func (c *AppConfig) RunQuery() (*QueryResult, error) {
}

b := &app.ReferenceBuilder{}
if err := AddComponentInputsToRefBuilder(b, &c.Resolvers, &c.ComponentInputs, &c.GenerationInputs); err != nil {
s := &c.SourceRepositories
i := &c.ImageStreams
if err := AddComponentInputsToRefBuilder(b, &c.Resolvers, &c.ComponentInputs, &c.GenerationInputs, s, i); err != nil {
return nil, err
}
components, repositories, errs := b.Result()
Expand Down
2 changes: 1 addition & 1 deletion pkg/oc/generate/app/cmd/newapp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func TestValidate(t *testing.T) {
continue
}

if err := AddComponentInputsToRefBuilder(b, &c.cfg.Resolvers, &c.cfg.ComponentInputs, &c.cfg.GenerationInputs); err != nil {
if err := AddComponentInputsToRefBuilder(b, &c.cfg.Resolvers, &c.cfg.ComponentInputs, &c.cfg.GenerationInputs, &c.cfg.SourceRepositories, &c.cfg.ImageStreams); err != nil {
t.Errorf("%s: Unexpected error: %v", n, err)
continue
}
Expand Down
30 changes: 21 additions & 9 deletions pkg/oc/generate/app/cmd/resolve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/openshift/origin/pkg/generate"
"github.com/openshift/origin/pkg/generate/app"
"github.com/openshift/origin/pkg/generate/git"
dockerfileutil "github.com/openshift/origin/pkg/util/docker/dockerfile"
)

Expand Down Expand Up @@ -99,9 +100,11 @@ func Resolve(appConfig *AppConfig) (*ResolvedComponents, error) {
r := &appConfig.Resolvers
c := &appConfig.ComponentInputs
g := &appConfig.GenerationInputs
s := &appConfig.SourceRepositories
i := &appConfig.ImageStreams
b := &app.ReferenceBuilder{}

if err := AddComponentInputsToRefBuilder(b, r, c, g); err != nil {
if err := AddComponentInputsToRefBuilder(b, r, c, g, s, i); err != nil {
return nil, err
}
components, repositories, errs := b.Result()
Expand Down Expand Up @@ -184,14 +187,25 @@ func Resolve(appConfig *AppConfig) (*ResolvedComponents, error) {

// AddSourceRepositoriesToRefBuilder adds the provided repositories to the reference builder, identifies which
// should be built using Docker, and then returns the full list of source repositories.
func AddSourceRepositoriesToRefBuilder(b *app.ReferenceBuilder, repos []string, g *GenerationInputs) (app.SourceRepositories, error) {
func AddSourceRepositoriesToRefBuilder(b *app.ReferenceBuilder, c *ComponentInputs, g *GenerationInputs, s, i *[]string) (app.SourceRepositories, error) {
strategy := g.Strategy
if strategy == generate.StrategyUnspecified {
strategy = generate.StrategySource
}
for _, s := range repos {
if repo, ok := b.AddSourceRepository(s, strategy); ok {
repo.SetContextDir(g.ContextDir)
// when git is installed we keep default logic. sourcelookup.go will do sorting of images, repos
if git.IsGitInstalled() || len(c.SourceRepositories) > 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if git is installed all logic in componentInputs and flag works. If it does not exist logic can recognize only URL and sets it as SourceRepository. If we pass hostDir - we need logic below.

for _, s := range c.SourceRepositories {
if repo, ok := b.AddSourceRepository(s, strategy); ok {
repo.SetContextDir(g.ContextDir)
}
}
// when git is not installed we need to parse some logic to decide if we got 'new-app -i image code' or 'image -c code' syntax
} else if len(c.Components) > 0 && len(*i) > 0 && len(*s) == 0 || len(c.Components) > 0 && len(*i) == 0 && len(*s) > 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This covers the case when half of the components are passed via flags, and half via Components. We need this because of the use-cases like "image image code" or "image image" where componentInputs becomes complex.

for _, s := range c.Components {
if repo, ok := b.AddSourceRepository(s, strategy); ok {
repo.SetContextDir(g.ContextDir)
c.Components = []string{}
}
}
}
if len(g.Dockerfile) > 0 {
Expand Down Expand Up @@ -263,18 +277,16 @@ func DetectSource(repositories []*app.SourceRepository, d app.Detector, g *Gener
}

// AddComponentInputsToRefBuilder set up the components to be used by the reference builder.
func AddComponentInputsToRefBuilder(b *app.ReferenceBuilder, r *Resolvers, c *ComponentInputs, g *GenerationInputs) error {
func AddComponentInputsToRefBuilder(b *app.ReferenceBuilder, r *Resolvers, c *ComponentInputs, g *GenerationInputs, s, i *[]string) error {
// lookup source repositories first (before processing the component inputs)
repositories, err := AddSourceRepositoriesToRefBuilder(b, c.SourceRepositories, g)
repositories, err := AddSourceRepositoriesToRefBuilder(b, c, g, s, i)
if err != nil {
return err
}

// identify the types of the provided source locations
if err := DetectSource(repositories, r.Detector, g); err != nil {
return err
}

b.AddComponents(c.DockerImages, func(input *app.ComponentInput) app.ComponentReference {
input.Argument = fmt.Sprintf("--docker-image=%q", input.From)
input.Searcher = r.DockerSearcher
Expand Down
40 changes: 40 additions & 0 deletions test/cmd/newapp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,5 +470,45 @@ os::cmd::expect_success_and_not_text 'oc new-app https://github.com/openshift/ru
# We permit running new-app against a remote URL which returns a template
os::cmd::expect_success 'oc new-app https://raw.githubusercontent.com/openshift/origin/master/examples/wordpress/template/wordpress-mysql.json --dry-run'

# new-app different syntax for new-app functionality
os::cmd::expect_success 'oc new-project new-app-syntax'
os::cmd::expect_success 'oc import-image openshift/ruby-20-centos7:latest --confirm'
os::cmd::expect_success 'oc import-image openshift/php-55-centos7:latest --confirm'
rm -rf ./test/testdata/testapp
git clone https://github.com/openshift/ruby-hello-world.git ./test/testdata/testapp
os::cmd::expect_success 'oc new-app ruby-20-centos7:latest~https://github.com/openshift/ruby-hello-world.git --dry-run'
os::cmd::expect_success 'oc new-app ruby-20-centos7:latest~./test/testdata/testapp --dry-run'
os::cmd::expect_success 'oc new-app -i ruby-20-centos7:latest https://github.com/openshift/ruby-hello-world.git --dry-run'
os::cmd::expect_success 'oc new-app -i ruby-20-centos7:latest ./test/testdata/testapp --dry-run'
os::cmd::expect_success 'oc new-app ruby-20-centos7:latest --code https://github.com/openshift/ruby-hello-world.git --dry-run'
os::cmd::expect_success 'oc new-app ruby-20-centos7:latest --code ./test/testdata/testapp --dry-run'
os::cmd::expect_success 'oc new-app -i ruby-20-centos7:latest --code https://github.com/openshift/ruby-hello-world.git --dry-run'
os::cmd::expect_success 'oc new-app -i ruby-20-centos7:latest --code ./test/testdata/testapp --dry-run'
Copy link
Contributor

Choose a reason for hiding this comment

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

in all these cases since we are supplying the builder image explicitly, we should expect that source detection will not be performed on the source argument. Add something to the test that confirms that:

  1. use a non-ruby image as the builder image and confirm the resulting build uses the non-ruby builder image (even though the source is ruby code)
  2. run new-app with a higher log level and check the output for indications that source detection is being performed (it should not be).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For point 2 code detection still happens, but the image is being used from the flags and not detection.
oc new-app php-55-centos7:latest --code ./app-examples/ruby --dry-run --loglevel=4
result: php

oc new-app --code ./app-examples/ruby --dry-run --loglevel=4
Result: ruby
Will do test to confirm.

Problem is that code detection is doing a little bit more than code detection so it needs to be executed[1]. We might want to split it. But I would not want to touch this one here as the logic to use flags values was already there.

[1]

I1215 20:51:28.617427   28984 resolve.go:254] checkig source code type for [./app-examples/ruby]
I1215 20:51:28.618698   28984 repository.go:385] Executing git config --get-regexp ^remote\..*\.url$
I1215 20:51:28.622574   28984 repository.go:385] Executing git symbolic-ref -q --short HEAD
I1215 20:51:28.625057   28984 sourcelookup.go:313] Checking if https://github.com/openshift/ruby-hello-world requires authentication
I1215 20:51:28.625074   28984 repository.go:385] Executing git ls-remote --heads https://github.com/openshift/ruby-hello-world

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found this strange behavior which I think is not as expected (its on 3.7 too and was not introduced by these code changes). It would be nice you could validate. Syntax are similar - one uses dicovery for images, other overwrites. BUT strategies in the result are different.

os::cmd::expect_success 'oc new-app --code ./test/testdata/testapp --name test'
os::cmd::expect_success_and_text 'oc get bc test --template={{.spec.strategy.dockerStrategy.from.name}}' 'ruby-22-centos7:latest'
os::cmd::expect_success 'oc new-app -i php-55-centos7:latest --code ./test/testdata/testapp --name test2'
os::cmd::expect_success_and_text 'oc get bc test2 --template={{.spec.strategy.sourceStrategy.from.name}}' 'php-55-centos7:latest'

Copy link
Contributor

Choose a reason for hiding this comment

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

what is in /test/testdata/testapp in the first test? if it contains a Dockerfile with FROM ruby-22-centos7:latest then i'd say it's behaving as expected.

Copy link
Contributor Author

@mjudeikis mjudeikis Dec 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

you are right :)

Copy link
Contributor

Choose a reason for hiding this comment

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

in all these cases since we are supplying the builder image explicitly, we should expect that source detection will not be performed on the source argument. Add something to the test that confirms that:

use a non-ruby image as the builder image and confirm the resulting build uses the non-ruby builder image
(even though the source is ruby code)

I don't see that this test was added for the various permutations of arguments (image~source, image source, -i image --source source, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was covered by 2 tests, but exteded with few more few more tests:

...
oc new-app php-55-centos7:latest --code https://github.com/openshift/ruby-hello-world.git --name test5
oc get bc test5 --template={{.spec.strategy.sourceStrategy.from.name}}
#result is php builder, despite the fact that code is ruby.
....

This covers all your cases. The only tests we don't have is how it behaves without git. This is due our testing infra limitation :/


os::cmd::expect_success 'oc new-app --code ./test/testdata/testapp --name test'
os::cmd::expect_success_and_text 'oc get bc test --template={{.spec.strategy.dockerStrategy.from.name}}' 'ruby-22-centos7:latest'

os::cmd::expect_success 'oc new-app -i php-55-centos7:latest --code ./test/testdata/testapp --name test2'
os::cmd::expect_success_and_text 'oc get bc test2 --template={{.spec.strategy.sourceStrategy.from.name}}' 'php-55-centos7:latest'

os::cmd::expect_success 'oc new-app -i php-55-centos7:latest~https://github.com/openshift/ruby-hello-world.git --name test3'
os::cmd::expect_success_and_text 'oc get bc test3 --template={{.spec.strategy.sourceStrategy.from.name}}' 'php-55-centos7:latest'

os::cmd::expect_success 'oc new-app php-55-centos7:latest~https://github.com/openshift/ruby-hello-world.git --name test4'
os::cmd::expect_success_and_text 'oc get bc test4 --template={{.spec.strategy.sourceStrategy.from.name}}' 'php-55-centos7:latest'

os::cmd::expect_success 'oc new-app -i php-55-centos7:latest https://github.com/openshift/ruby-hello-world.git --name test5'
os::cmd::expect_success_and_text 'oc get bc test5 --template={{.spec.strategy.sourceStrategy.from.name}}' 'php-55-centos7:latest'

os::cmd::expect_success 'oc new-app php-55-centos7:latest --code https://github.com/openshift/ruby-hello-world.git --name test6'
os::cmd::expect_success_and_text 'oc get bc test6 --template={{.spec.strategy.sourceStrategy.from.name}}' 'php-55-centos7:latest'

os::cmd::expect_success 'oc new-app https://github.com/openshift/ruby-hello-world.git --name test7'
os::cmd::expect_success_and_text 'oc get bc test7 --template={{.spec.strategy.dockerStrategy.from.name}}' 'ruby-22-centos7:latest'

os::cmd::expect_success 'oc new-app php-55-centos7:latest https://github.com/openshift/ruby-hello-world.git --name test8'
os::cmd::expect_success_and_text 'oc get bc test8 --template={{.spec.strategy.sourceStrategy.from.name}}' 'php-55-centos7:latest'
os::cmd::expect_success 'oc delete project new-app-syntax'

echo "new-app: ok"
os::test::junit::declare_suite_end