Updates deploy to build API #454

Cawllec · 2018-01-10T15:56:35Z

Updates to the new Bugsnag build API.

Adds build method to client to send build information off to bugsnag
Points clients deploy method to build method
Maintains old deploy method in HTTP client unchanged
Adds new sendBuildReport to HTTP client to handle new API calls.
Adds setBuildEndpoint and getBuildEndpoint to configuration, with a default.

…-php into cawllec/update-build-api

kattrali · 2018-01-10T17:57:30Z

src/Client.php

@@ -309,7 +309,7 @@ public function notify(Report $report, callable $callback = null)
    }

    /**
-     * Notify Bugsnag of a deployment.
+     * Notify Bugsnag of a deployment. This function is being deprecated in favour of `build`.


Add the @deprecated annotation at the beginning of this line. Then it will also be picked up by API documentation/linting.

kattrali · 2018-01-10T17:58:12Z

src/HttpClient.php

@@ -71,6 +71,7 @@ public function queue(Report $report)

    /**
     * Notify Bugsnag of a deployment.
+     * This method should no longer be used in favour of sendBuildReport.


Same as above, add @deprecated

kattrali · 2018-01-10T17:59:57Z

tests/ClientTest.php

+        $this->client->build('baz', 'foo', 'github', 'me');
+    }
+
+    public function testBuildAutoDetectsProvider()


There should be tests for the other providers as well

…-php into cawllec/update-build-api

snmaynard · 2018-01-10T18:53:32Z

src/HttpClient.php

+            $sourceControl['provider'] = $buildInfo['provider'];
+        } elseif (isset($buildInfo['repository'])) {
+            $url = $buildInfo['repository'];
+            if (strpos($url, 'github.com') != false) {


You dont need to do this. Provider is optional

Every field is optional though. Are we using this field for something?

We shouldnt automatically set this in any of our sdks. This logic of guessing the provider is on the server (and therefore is easier to update).

The user needs to be able to set it for when they are using an on-premise installation of github and the repository URL isnt the github.com one.

It gives us the extra data they may not set otherwise.

Ah, ok, didn't see this.

snmaynard · 2018-01-10T18:54:33Z

src/HttpClient.php

+        }
+
+        if (isset($buildInfo['buildTool'])) {
+            $data['buildTool'] = $buildInfo['buildTool'];


You shouldnt be able to externally set this

Its settable for laravel to override

I dont think we care about php/laravel distinction at an analytics level. We just care that this lib is being used for deploys.

snmaynard · 2018-01-10T18:57:24Z

src/HttpClient.php

+        }
+
+        if (isset($buildInfo['builder'])) {
+            $data['builderName'] = $buildInfo['builder'];


This should default to whoami or a php cross platform equivalent.

I don't think we want to add shell calls in the base notifier, it's much safer to use wrapped commands in higher level libs.

Why not? If this isnt automated very few people will set it.

You probably want:

get_current_user();

instead of shell executing whoami. That function will give you the owner of the file, which is maybe better than nothing.

Something you could do if the user has installed and not disabled the functions from posix extension, is:

posix_getpwuid(posix_geteuid())['name'];

whoami works across all windows post server-2003 & unix-like systems. I'll add a check on the response to see if it returned correctly and fallback to get_current_user if not.

GrahamCampbell · 2018-01-10T22:52:48Z

src/HttpClient.php

+        if (isset($buildInfo['builder'])) {
+            $data['builderName'] = $buildInfo['builder'];
+        } else {
+            $data['builderName'] = exec('whoami');


You should check if this function has been disabled, otherwise it'll crash. Many web hosts disable this by default for security.

GrahamCampbell · 2018-01-10T22:52:59Z

src/HttpClient.php

+        }
+
+        if (isset($buildInfo['builder'])) {
+            $data['builderName'] = $buildInfo['builder'];


You probably want:

get_current_user();

instead of shell executing whoami. That function will give you the owner of the file, which is maybe better than nothing.

Something you could do if the user has installed and not disabled the functions from posix extension, is:

posix_getpwuid(posix_geteuid())['name'];

…-php into cawllec/update-build-api

jbrooksuk · 2018-01-16T11:17:46Z

I was just looking for this and was about to contribute it. Is there any chance we can see this get merged and released soon?

Cawllec · 2018-01-16T11:18:38Z

Yeah, i'll get this released this afternoon if possible.

jbrooksuk · 2018-01-16T11:19:40Z

Awesome, thanks @Cawllec!

adjustments made to fallback to get_file_owner

Cawllec and others added 4 commits January 10, 2018 15:16

Updated build command, keeping old methods available

0784a3f

Apply fixes from StyleCI (#453)

1d5bfe4

Added buildername to build parameters

fefd5af

Merge branch 'cawllec/update-build-api' of github.com:bugsnag/bugsnag…

a4e4f5e

…-php into cawllec/update-build-api

Cawllec requested a review from kattrali January 10, 2018 15:56

kattrali reviewed Jan 10, 2018

View reviewed changes

Cawllec and others added 4 commits January 10, 2018 18:13

Added deprecated tags, added provider tests

1615d68

Apply fixes from StyleCI (#455)

f26f6a1

Added build-tool param

3c6537c

Merge branch 'cawllec/update-build-api' of github.com:bugsnag/bugsnag…

caea76f

…-php into cawllec/update-build-api

kattrali approved these changes Jan 10, 2018

View reviewed changes

snmaynard reviewed Jan 10, 2018

View reviewed changes

Changes to api

2216988

GrahamCampbell previously requested changes Jan 10, 2018

View reviewed changes

Cawllec and others added 6 commits January 10, 2018 23:48

Added check on exec function

a34a51c

Apply fixes from StyleCI (#456)

a996030

Improved user detection, created static utils to avoid class pollution

f62133b

Merge branch 'cawllec/update-build-api' of github.com:bugsnag/bugsnag…

0277fe4

…-php into cawllec/update-build-api

Apply fixes from StyleCI (#457)

b910c53

Fixed param err

a47ac7d

kattrali merged commit 4c66b9a into master Jan 18, 2018

kattrali deleted the cawllec/update-build-api branch January 18, 2018 22:35

imjoehaines mentioned this pull request Jun 4, 2020

Use HttpClient in SessionTracker #582

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates deploy to build API #454

Updates deploy to build API #454

Cawllec commented Jan 10, 2018

kattrali Jan 10, 2018

kattrali Jan 10, 2018

kattrali Jan 10, 2018

snmaynard Jan 10, 2018

kattrali Jan 10, 2018

snmaynard Jan 10, 2018

Cawllec Jan 10, 2018

Cawllec Jan 10, 2018

snmaynard Jan 10, 2018

kattrali Jan 10, 2018

snmaynard Jan 10, 2018

snmaynard Jan 10, 2018

Cawllec Jan 10, 2018

snmaynard Jan 10, 2018

GrahamCampbell Jan 10, 2018

Cawllec Jan 11, 2018

GrahamCampbell Jan 10, 2018

GrahamCampbell Jan 10, 2018

jbrooksuk commented Jan 16, 2018

Cawllec commented Jan 16, 2018

jbrooksuk commented Jan 16, 2018

Updates deploy to build API #454

Updates deploy to build API #454

Conversation

Cawllec commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrooksuk commented Jan 16, 2018

Cawllec commented Jan 16, 2018

jbrooksuk commented Jan 16, 2018