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

HTTPProjectConfigManager: datafile should be a string #243

Merged
merged 2 commits into from
May 12, 2022

Conversation

adri
Copy link
Contributor

@adri adri commented Apr 14, 2022

Summary

  • By using ->getContents() the actual string is returned instead of a Guzzle Stream object. The interface uses string everywhere.

By using `->getContents()` the actual string is returned instead of a Guzzle Stream object. The interface uses string everywhere.
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

changes seem fine to me. Let's do our internal testing and will merge if it is passed.

@adri
Copy link
Contributor Author

adri commented May 3, 2022

Thanks a lot for looking into this @msohailhussain. I tested it in our project. Is there already and update on internal testing? :) Thanks again!

@msohailhussain
Copy link
Contributor

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

PR looks good, please update the Copyright header.

@msohailhussain
Copy link
Contributor

@adri waiting for CLA and one change request. Also update the branch.

@adri
Copy link
Contributor Author

adri commented May 11, 2022

@msohailhussain Done!

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm!!! need to update copyright header.

@msohailhussain msohailhussain merged commit 95cea9b into optimizely:master May 12, 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.

3 participants