search

mattermost / mattermost-plugin-starter-template

Build scripts and templates for writing Mattermost plugins.
https://developers.mattermost.com/extend/plugins/
Apache License 2.0
133 stars 121 forks source link

changed single expression plugin upload to validate responses #36

Closed scottleedavis closed 5 years ago

scottleedavis commented 5 years ago

Instead of individual calls '&&' together, the deploy now checks each response for a valid result.

This fixes problems on Ubuntu 16.04 when mysteriously, the && && ok || failed never works.

lieut-data commented 5 years ago

@scottleedavis, can you give some context into the original issue? I'd like to reproduce it to try to better understand the fix here.

scottleedavis commented 5 years ago

Sure @lieut-data.

I noticed my ThinkPad/Ubuntu16.04 laptop never seemed to upload plugins via 'make deploy' with the mattermost-preview docker image, and I assumed I had something setup incorrectly. Later at some point, I setup the server and webapp locally and the same issue happened... That of an apparent passed upload from the Makefile, but no logs or uploaded plugin resultant.

I mentioned this to ~extentions and @hanzei encouraged me to dig a bit deeper. During which I found that by separating the api calls in the Makefile, I was then able to see 100% success in uploads.

I have never had this problem previously on OSX.

lieut-data commented 5 years ago

Agree with @levb: I think we need to suss out the root issue here. @scottleedavis, can you share the output from make deploy when this fails?

scottleedavis commented 5 years ago

Here is the failed output of make deploy which produces no visible error in output.

plugin built at: dist/com.mattermost.sample-plugin-0.1.0.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    45    0     0  100    45      0  55900 --:--:-- --:--:-- --:--:-- 45000
Installing plugin via API
OK.

I used mitmproxy to capture the HTTP failure results

>> POST http://localhost:8065//api/v4/users/login
        ← 301 [no content] 27ms
   POST http://localhost:8065//api/v4/plugins
        ← [Errno 32] Broken pipe
   POST http://localhost:8065//api/v4/plugins/com.mattermost.sample-plugin/enable
        ← 301 [no content] 53ms

As can be seen by looking at the proxy calls, the double slash '//', is breaking the request by curl, and when I reviewed the ENV variable MM_SERVICESETTINGS_SITEURL=http://localhost:8065/ it had the trailing slash.

By removing the trailing '/' in the variable, all plugins deploy as expected.

This was my own error in not recognizing that I likely updated the environment variable manually while developing and testing for this PR.

lieut-data commented 5 years ago

@scottleedavis, what if we added the --location and --post301 flags to each curl invocation, telling it to follow redirects and not convert the POST back to a GET on doing so?

scottleedavis commented 5 years ago

@lieut-data that appears to solve the issue. It does however break the mitmproxy, which isn't a problem imho.

lieut-data commented 5 years ago

Sounds good -- should we revert to the original invocation as such for now then (plus the new flags)?

scottleedavis commented 5 years ago

@lieut-data I feel we should and went ahead and pushed a commit. However, if @levb or @hanzei feel we should use the updated approach of checking responses I am also open to that.

lieut-data commented 5 years ago

@levb, I'm going to go ahead and merge this since we sussed out the root issue here as per your original feedback.