Skip to content
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.

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

automate-ui: cleanup statusText usage #698

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Jun 27, 2019

  • removed from test specs where it didn't matter
  • replaced with error.message where it did matter
  • placed a comment on it where I didn't want to change anything

What is this? HTTP/2 doesn't send a "status text". Status text is the last segment after

HTTP/x.y <CODE> <REASON>

in the server response, so, where with HTTP/1.1 we see

HTTP/1.1 401 Unauthorized

with HTTP/2, we see

HTTP/2 401

The corresponding RFC7540 paragraph is

HTTP/2 does not define a way to carry the version or reason phrase that is included in an HTTP/1.1 status line.

@srenatus srenatus added bug 🐛 Something isn't working automate-ui labels Jun 27, 2019
@srenatus srenatus self-assigned this Jun 27, 2019
@srenatus srenatus force-pushed the sr/automate-ui/cleanup-usage-of-statusText branch 2 times, most recently from dcf9653 to 237e789 Compare June 27, 2019 13:24
@srenatus srenatus added the ui label Jun 27, 2019
Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

👍 👍

Copy link
Contributor

@msorens msorens left a comment

Choose a reason for hiding this comment

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

I believe the fix applied in telemetry.service.ts also needs to be applied here:

(1) telemetry.service.ts::L262

console.log(`Error emitting telemetry event: ${error.status} - ${error.statusText}`);

(2) reset_password.component.ts::L53

error.status ? `${error.status} - ${error.statusText}` : 'Server error';

@srenatus
Copy link
Contributor Author

srenatus commented Jun 28, 2019

reset_password.component.ts ooooh workflow. :shiver: ;) I'm not going to touch this. It's completely slipped my mind how to test that, and I don't want to fiddle with it. Also, I'm not sure if its password reset stuff even still makes sense or is usable.

- removed from test specs where it didn't matter
- replaced with error.message where it did matter
- placed a comment on it where I didn't want to change anything

What is this? HTTP/2 doesn't send a "status text". Status text is the last segment after

    HTTP/x.y <CODE> <REASON>

in the server response, so, where with HTTP/1.1 we see

    HTTP/1.1 401 Unauthorized

with HTTP/2, we see

    HTTP/2 401

The corresponding RFC7540 paragraph is [1]:

> HTTP/2 does not define a way to carry the version or reason phrase that is included in an HTTP/1.1 status line.

[1]: https://greenbytes.de/tech/webdav/rfc7540.html#rfc.section.8.1.2.4.p.2

Signed-off-by: Stephan Renatus <[email protected]>
@srenatus srenatus force-pushed the sr/automate-ui/cleanup-usage-of-statusText branch from 237e789 to 826c862 Compare June 28, 2019 08:08
@srenatus
Copy link
Contributor Author

WMWG (will merge when green)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automate-ui bug 🐛 Something isn't working ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants