Sealights integration fix#443
Conversation
|
@RoniSegal our team has confirmed that the changes work fine when we use the buildpack URL as https://github.com/victor-katsovsky/nodejs-buildpack.git Thanks for fixing this issue. |
| PushAppAndConfirm(app) | ||
| Expect(RunCF("bind-service", app.Name, serviceNameOne)).To(Succeed()) | ||
| PushAppAndConfirm(app) |
There was a problem hiding this comment.
Is there a reason this app is pushed twice as opposed to something more idiomatic like:
Expect(app.PushNoStart()).To(Succeed())
Expect(RunCF("bind-service", app.Name, serviceNameOne)).To(Succeed())
Expect(app.Restart()).To(Succeed())
If not, can you change it?
| PushAppAndConfirm(app) | ||
| Expect(RunCF("bind-service", app.Name, serviceNameOne)).To(Succeed()) | ||
| PushAppAndConfirm(app) | ||
| Expect(app.DownloadDroplet(filepath.Join(app.Path, "droplet.tgz"))).To(Succeed()) |
There was a problem hiding this comment.
This droplet.tgz is left hanging within the fixtures after the test. Can you clean it up? It would be great if you can use a tempfile instead of downloading it into the the fixture/<app> dir.
| var sb strings.Builder | ||
| sb.WriteString("./node_modules/.bin/slnodejs run --useinitialcolor true ") | ||
|
|
||
| if o.TokenFile != "" { |
There was a problem hiding this comment.
Not sure if I'm missing something - I couldn't find a place where TokenFile is populated but I see its value being checked here and in other places
| sl.Log.Info("Sealights token not found") | ||
| return fmt.Errorf(EmptyTokenError), "" | ||
| } | ||
| split := strings.SplitAfter(originalCommand, "node") |
There was a problem hiding this comment.
Is it always assured that a user who wishes to use the sealights integration would always set their start command (say in a Procfile) as node ...? Can they set something like npm start? If that's not relevant for your case, this is fine with me. If that's a use-case left out in error, can you handle it?
| } | ||
| split := strings.SplitAfter(originalCommand, "node") | ||
|
|
||
| o := sl.getSealightsOptions(split[1], token) |
There was a problem hiding this comment.
Can you verify the length of split[] before using the index and otherwise log a readable error?
1. Assign TokenFile property 2. Added fallback for npm commands 3. Remove temp droplet file after the tests
Refactoring after review
|
Thanks @arjun024, fixed your comments |
5 participants
Thanks for contributing to the buildpack. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change:
An explanation of the use cases your change solves
I have viewed signed and have submitted the Contributor License Agreement
I have made this pull request to the
developbranchI have added an integration test