Fix create-app-manifest only includes one host [92530254] #425

SrinivasChilveri · 2015-04-24T07:00:14Z

After fixing the issue cli create-app-manifest will cosiders
hosts,domains based on multiple routes to an app,so when ever
has more then one host or domain uses the hosts & domains
key word respectively.
the changed code doesn't have perf impact if an app has single route...
story: #92530254
Any comments/suggestions are welcome. From my side i have some other options as follows

Option1: I can remove the below code which hits on single route case because which has taken care in writeRoutesToFile but will have little less performance.

if len(app.Routes) == 1 {
if _, err := fmt.Fprintf(f, " host: %s\n", app.Routes[0].Host); err != nil {
return err
}
if _, err := fmt.Fprintf(f, " domain: %s\n", app.Routes[0].Domain.Name); err != nil {
return err
}
}

Option 2: Option 1 & always use only the hosts & domains keys in manifest.yml file so i can remove below code from writeRoutesToFile & change the required test cases

if len(hosts) == 1 {
if _, err := fmt.Fprintf(f, " host: %s\n", hosts[0]); err != nil {
return err
}
}
if len(domains) == 1 {
if _, err := fmt.Fprintf(f, " domain: %s\n", domains[0]); err != nil {
return err
}
}

cfdreddbot · 2015-04-24T07:00:14Z

Hey SrinivasChilveri!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

cf-gitbot · 2015-04-24T07:00:17Z

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/93178282.

maximilien · 2015-04-24T22:07:29Z

@SrinivasChilveri just FYI that when pasting code (Golang, Ruby, etc or even JSON and YAML) if you use the Github Markdown within ``` the resulting code will keep it's formatting. You can even tell it what language and it will syntax highlight. This will help in many communications and keep things clearer IMO, so for instance, cutting and pasting part of your comment above would result in:

[...]
if len(app.Routes) == 1 {
  if _, err := fmt.Fprintf(f, " host: %s\n", app.Routes[0].Host); err != nil {
    return err
  }
[...]
}

If you click on the "Markdown supported" link on each Github comment box you can get details.

Finally, just a matter of style, since we are on the subject, I would write code above as:

if len(app.Routes) == 1 {
  _, err := fmt.Fprintf(f, " host: %s\n", app.Routes[0].Host); 
  if err != nil {
    return err
  }
  [...]
}

Which I think is clearer. More lines but easier to parse (for humans).

OK, now back to the regularly scheduled reviewing session.

maximilien · 2015-04-24T22:09:30Z

cf/manifest/generate_manifest.go

-			}
-			if _, err := fmt.Fprintf(f, "  domain: %s\n", app.Routes[0].Domain.Name); err != nil {
-				return err
+


get rid of unnecessary new line here and other such places.

SrinivasChilveri · 2015-04-28T05:17:26Z

Hi @maximilien , Thanks a lot for review comments. The comments are fixed.I have not fixed if style comment because i had followed the style which was there in that file.Any other suggestions are welcome.

jberkhahn · 2015-06-05T00:50:35Z

Yeah I agree with max's comment, could you separate the single context in the test into multiple contexts like "multiple host, multiple domains", "single host, multiple domains", etc.

Would make the test much easier to parse.

goehmen · 2015-06-09T18:00:43Z

@SrinivasChilveri The story to pull in this PR is at the top of the LCI backlog. Can you separate the single context in to multiple as per @jberkhahn ? thx

SrinivasChilveri · 2015-06-15T11:24:07Z

Hi @goehmen , Sorry for late reply because i was on leave. Any how i have changed the test cases as suggested & updated but CI is failing because of below reason:

147# code.google.com/p/go.tools/cmd/vet

148../../../code.google.com/p/go.tools/cmd/vet/shadow.go:216: not enough arguments in call to obj.Parent().Parent().LookupParent

149../../../code.google.com/p/go.tools/cmd/vet/types.go:21: undefined: types.New

150../../../code.google.com/p/go.tools/cmd/vet/types.go:22: undefined: types.New

151../../../code.google.com/p/go.tools/cmd/vet/types.go:23: undefined: types.New

152

153The command "go get -v code.google.com/p/go.tools/cmd/vet" failed and exited with 2 during .

154

155Your build has been stopped.

Any suggestions are welcome.

Thanks & Regards,
SrinivasChilveri

After fixing the issue cli create-app-manifest will cosiders hosts,domains based on multiple routes to an app,so when ever has more then one host or domain uses the hosts & domains key word respectively. the changed code doesn't have perf impact if an app has single route Fixed Review Comments Fixed review comments

SrinivasChilveri · 2015-06-17T07:19:28Z

Hi @jberkhahn
Apologies for the delay in responding. i was on leave.Any how i have changed the test cases as suggested & updated but CI is failing because of below reason:

147# code.google.com/p/go.tools/cmd/vet

148../../../code.google.com/p/go.tools/cmd/vet/shadow.go:216: not enough arguments in call to obj.Parent().Parent().LookupParent

149../../../code.google.com/p/go.tools/cmd/vet/types.go:21: undefined: types.New

150../../../code.google.com/p/go.tools/cmd/vet/types.go:22: undefined: types.New

151../../../code.google.com/p/go.tools/cmd/vet/types.go:23: undefined: types.New

152

153The command "go get -v code.google.com/p/go.tools/cmd/vet" failed and exited with 2 during .

154

155Your build has been stopped.

Any suggestions are welcome.

Thanks & Regards,
SrinivasChilveri

goehmen · 2015-06-18T05:39:43Z

@SrinivasChilveri Yes - we're working on the Travis issue.

simonleung8 · 2015-07-10T21:42:06Z

cf/manifest/generate_manifest_test.go

+
+		It("generates a manifest containing two hosts two domains", func() {
+			m.Memory("app1", 128)
+			m.StartupCommand("app1", "run main.go")


StartupCommand doesn't seem to be a thing, not defined anywhere

simonleung8 · 2015-07-10T21:42:58Z

We pulled and tried this PR, but it doesn't pass the unit tests.
Can you fix up the error and we will take another look.

~Thanks

SrinivasChilveri · 2015-07-12T10:10:23Z

Hi @simonleung8 , I ran all test cases locally with the branch, its working fine. I am not sure where is wrong, can you please provide more info. The StartupCommand is part of AppManifest as shown below.

The type AppManifest interface {
Memory(string, int64)
Service(string, string)
StartupCommand(string, string)
EnvironmentVars(string, string, string)
HealthCheckTimeout(string, int)
Instances(string, int)
Domain(string, string, string)
GetContents() []models.Application
FileSavePath(string)
Save() error
}

Thanks & Regards,
SriniCH

simonleung8 · 2015-07-14T22:19:36Z

Hi, looks like this PR needs to be rebased.
StartupCommand has been renamed, see https://github.com/cloudfoundry/cli/blob/master/cf/manifest/generate_manifest.go#L14

It is not showing you the PR status since travis has failed, you could close and open a new PR to get a green travis if you want, or we can just work out of this one, it is up to you.

SrinivasChilveri · 2015-07-15T08:58:34Z

Hi @simonleung8 , I have raised new PR #514 , so we can close this PR

maximilien reviewed Apr 24, 2015
View reviewed changes

SrinivasChilveri force-pushed the hwcf-issue-31 branch from ac2bc02 to f6586c0 Compare April 28, 2015 05:01

goehmen mentioned this pull request Apr 28, 2015

create-app-manifest only includes one host #414

Closed

SrinivasChilveri force-pushed the hwcf-issue-31 branch from 0760c23 to 6858561 Compare June 15, 2015 11:02

simonleung8 reviewed Jul 10, 2015
View reviewed changes

camelpunch closed this Sep 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix create-app-manifest only includes one host [92530254] #425

Fix create-app-manifest only includes one host [92530254] #425

SrinivasChilveri commented Apr 24, 2015

cfdreddbot commented Apr 24, 2015

cf-gitbot commented Apr 24, 2015

maximilien commented Apr 24, 2015

maximilien Apr 24, 2015

SrinivasChilveri commented Apr 28, 2015

jberkhahn commented Jun 5, 2015

goehmen commented Jun 9, 2015

SrinivasChilveri commented Jun 15, 2015

SrinivasChilveri commented Jun 17, 2015

goehmen commented Jun 18, 2015

simonleung8 Jul 10, 2015

simonleung8 commented Jul 10, 2015

SrinivasChilveri commented Jul 12, 2015

simonleung8 commented Jul 14, 2015

SrinivasChilveri commented Jul 15, 2015

Fix create-app-manifest only includes one host [92530254] #425

Fix create-app-manifest only includes one host [92530254] #425

Conversation

SrinivasChilveri commented Apr 24, 2015

cfdreddbot commented Apr 24, 2015

cf-gitbot commented Apr 24, 2015

maximilien commented Apr 24, 2015

maximilien Apr 24, 2015

Choose a reason for hiding this comment

SrinivasChilveri commented Apr 28, 2015

jberkhahn commented Jun 5, 2015

goehmen commented Jun 9, 2015

SrinivasChilveri commented Jun 15, 2015

SrinivasChilveri commented Jun 17, 2015

goehmen commented Jun 18, 2015

simonleung8 Jul 10, 2015

Choose a reason for hiding this comment

simonleung8 commented Jul 10, 2015

SrinivasChilveri commented Jul 12, 2015

simonleung8 commented Jul 14, 2015

SrinivasChilveri commented Jul 15, 2015