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

#1705 Add UUID attribute to welcome message #1708

Merged
merged 9 commits into from Nov 8, 2018

Conversation

@Ryan-Gordon
Copy link
Contributor

@Ryan-Gordon Ryan-Gordon commented Nov 2, 2018

couch_server convienently already has a get_uuid() function located here : https://github.com/apache/couchdb/blob/master/src/couch/src/couch_server.erl#L18

Overview

Related to issue #1705
#1705

From what I could see, the functionality for this was already in place. Just needed to be added to the payload.
Let me know if I need to change some documentation or modify a test.

Testing recommendations

The base welcome message located at GET / should now have a UUID returned

Related Issues or Pull Requests

#1705

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
src/chttpd/src/chttpd_misc.erl Outdated Show resolved Hide resolved
@iilyak
Copy link
Contributor

@iilyak iilyak commented Nov 2, 2018

The tests for welcome message are here https://github.com/apache/couchdb/blob/master/src/chttpd/test/chttpd_welcome_test.erl

Tests can be run individually using

make eunit apps=chttpd tests=welcome_test_
Turns out get_uuid() already returns binary so converting again to binary with list_to_binary on a binary argument causing badarg exception as pointed out by @iilyak.
Ran tests with 2 passing on my side
@Ryan-Gordon
Copy link
Contributor Author

@Ryan-Gordon Ryan-Gordon commented Nov 2, 2018

Thank you @iilyak for your suggestion and help. I removed the list_to_binary conversion and now all tests pass!

@iilyak
iilyak approved these changes Nov 2, 2018
Copy link
Contributor

@iilyak iilyak left a comment

+1

@wohali
Copy link
Member

@wohali wohali commented Nov 2, 2018

@Ryan-Gordon Thank you very much for your first contribution to CouchDB! 🎊

It would be nice if you could add a new test to the test suite that actually confirms the UUID is included before we merge this.

@iilyak
Copy link
Contributor

@iilyak iilyak commented Nov 2, 2018

@Ryan-Gordon in addition to new test please squash commits into one, before we merge. Thank you for you contribution. Please don't hesitate to reach out to me if you would have any difficulties with writing the test case.

Uuida gathered with couch_util:get_value(<<"uuid">>, Json, undefined)
RealUuid gathered with
RealUuid = couch_server:get_version()
Assert they are equal
@Ryan-Gordon
Copy link
Contributor Author

@Ryan-Gordon Ryan-Gordon commented Nov 2, 2018

Hello 👍

I just wrote a test case for this UUID which is very similar to the should_have_version test in the same file. If the tests pass happy to have the commits squashed into 1

Ryan-Gordon added 2 commits Nov 2, 2018
Couch_server had this functionality already.

Added a test to ensure this works
@Ryan-Gordon
Copy link
Contributor Author

@Ryan-Gordon Ryan-Gordon commented Nov 2, 2018

@iilyak So I tried to squash on my own side and what I managed was rolling back 3 commits, committing as one and then a 2nd commit to merge branch

Ryan-Gordon added 3 commits Nov 2, 2018
Couch_server had this functionality already.

Added a test to ensure this works. This gets the UUID and then compares to what is in the welcome message
@wohali
Copy link
Member

@wohali wohali commented Nov 2, 2018

@Ryan-Gordon Don't worry about the squashing, I can do that when I merge your changes into master with the big "Squash and merge" button. But if you'd like help on git commands, come visit #couchdb-dev on Freenode IRC and someone can walk you through it. Or, this tutorial might help, it's the one I learned from originally.

Looks like your test passed! Are you ready for me to merge this now?

@Ryan-Gordon
Copy link
Contributor Author

@Ryan-Gordon Ryan-Gordon commented Nov 2, 2018

Ahhh yes, the Squash and merge is what im most familiar with !
Cool I'll check out the tutorial and maybe join the IRC anyways
Yes all ready to merge !
@wohali

@wohali wohali added this to the 2.3.0 milestone Nov 8, 2018
@wohali wohali merged commit c157c96 into apache:master Nov 8, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wohali
Copy link
Member

@wohali wohali commented Nov 8, 2018

@Ryan-Gordon All done, and thanks again! 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants