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

add username feature #126

Merged
merged 7 commits into from
Sep 2, 2021
Merged

add username feature #126

merged 7 commits into from
Sep 2, 2021

Conversation

utsavtulsyan
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2021

CLA assistant check
All committers have signed the CLA.

@carlossscastro carlossscastro added feature request Categorizes issue or PR as related to a new feature or enhancement. triage/pending Issue or PR is pending for triage and prioritization. labels Aug 16, 2021
@davidgit davidgit linked an issue Aug 20, 2021 that may be closed by this pull request
@roobre roobre removed the triage/pending Issue or PR is pending for triage and prioritization. label Aug 20, 2021
@roobre
Copy link
Contributor

roobre commented Aug 20, 2021

Thanks a lot for submitting this! We'll try to provide some feedback as soon as possible :)

@carlossscastro carlossscastro added priority/short-term Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Aug 20, 2021
@roobre
Copy link
Contributor

roobre commented Aug 23, 2021

Hi there @utsavtulsyan!

So far this is looking nice to me. The only thing I'd miss is covering this use case with an integration test, to ensure we won't break this by accident in future updates.

We have an skeleton for running integration tests using docker in the integration folder, and a fairly simple test suite written in go in the same folder. Please try to take a look and tell us if you'd feel confortably by adding a case for username+password authentication there, in a similar fashion that the one we have for TLS.

If you find it too convoluted just tell us and we'll try to prioritize adding this on our side :)

BR,
Roberto

@utsavtulsyan
Copy link
Contributor Author

Hi @roobre,
I have added the integration test as suggested, please have a look.
Regards,
Utsav

@roobre roobre requested a review from a team August 24, 2021 15:30
@roobre
Copy link
Contributor

roobre commented Aug 24, 2021

Thanks a lot @utsavtulsyan!
I've pinged the team for revew, we'll try to leave some feedback as soon as possible! Overall, however, PR is looking good to me so I wouldn't expect for this to take too long or require many changes.

Copy link
Contributor

@carlossscastro carlossscastro left a comment

Choose a reason for hiding this comment

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

This all looks good @utsavtulsyan
Thanks for the PR.
We'll release it shortly

👍

@carlossscastro carlossscastro merged commit 30a8c70 into newrelic:master Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Categorizes issue or PR as related to a new feature or enhancement. priority/short-term Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACL based authorization
4 participants