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

server: Begin accepting auth header and have it override callback URL values #2357

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

thomshutt
Copy link
Contributor

@thomshutt thomshutt commented Apr 8, 2022

What does this pull request do? Explain your changes. (required)

  • Begin accepting a new Livepeer-Transcode-Configuration header in the HTTP Push segment endpoint.
  • If present, this header should have the same format as the response from the "HTTP Auth Callback URL".
  • If present and the Manifest ID from the path is not referring to an existing Manifest ID, then a call is still made to the callback URL as before (and fails if this call returns an error) but the fields it returns are overridden by the ones in the header
  • If present and the Manifest ID in the URL already exists, this field is ignored

Specific updates (required)

  • Create a new method for parsing the header and associated unit tests
  • Add logic to override response from callback URL in the case outlined above. Unit test for this override behavior.

How did you test each of these updates (required)
✅ Wrote new unit tests
✅ TODO: Manual testing

Does this pull request close any open issues?

Checklist:

Copy link
Contributor

@oscar-davids oscar-davids left a comment

Choose a reason for hiding this comment

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

LGTM!

return true
}

profilesA, err := json.Marshal(a.Profiles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: is it ok to suppress error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can't see any actual situation where this wouldn't marshal but probably good practice to do something with the error anyway

Copy link
Contributor

@cyberj0g cyberj0g left a comment

Choose a reason for hiding this comment

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

LGTM!

@thomshutt thomshutt merged commit f2ec58d into master Apr 21, 2022
@thomshutt thomshutt deleted the auth-header-2 branch April 21, 2022 08:52
ad-astra-video pushed a commit to ad-astra-video/go-livepeer that referenced this pull request May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants