fix: consider local-ip hosts as local (#769)#806
Conversation
|
@santhosh-7777 Thank you for working on this. Could you please add a simple test to ensure that it works? |
|
@binokaryg Added a test to cover local-ip hostname handling. Please take another look. Thanks! |
binokaryg
left a comment
binokaryg
left a comment
There was a problem hiding this comment.
I've requested some changes.
| expect(actual).to.equal('abc'); | ||
| }); | ||
|
|
||
| it('returns matching snapshot with specified db name for local-ip hostname', () => { |
There was a problem hiding this comment.
Nit: the blank line between the old localhost test and your new test was removed. The rest of the file keeps a blank line between it(...) blocks. Add one back for consistency.
There was a problem hiding this comment.
Please add a single blank line between lines 118 and 119.
| sinon.stub(readline, 'keyInSelect').returns(2); | ||
| sinon.stub(apiStub.db, 'get').resolves({ _id: 'a', _rev: 'x', value: 1 }); | ||
| sinon.stub(fs, 'read').returns(JSON.stringify({ a: { 'localhost/medic': 'y' }})); | ||
| sinon.stub(fs, 'read').returns(JSON.stringify({ a: { 'localhost/medic': 'y' } })); |
There was a problem hiding this comment.
I think these are auto-format-on-save changes.
Although they look valid, they create noise and deviates from the PR scope. If you want to update them, please feel free to open a new PR for them.
|
Hi @binokaryg, I've addressed both review comments added the blank line back between the tests and reverted the auto format changes. Ready for re review when you get a chance. Thanks! |
There are more of those formatting changes. Can you please revert them all? |
|
Hi @binokaryg, I’ve reverted the remaining formatting only changes and kept only the required test addition for the local ip hostname case. Please take another look when you have time. Thanks! |
binokaryg
left a comment
binokaryg
left a comment
There was a problem hiding this comment.
I'd requested to add a single between between two it blocks.
| expect(actual).to.equal('abc'); | ||
| }); | ||
|
|
||
| it('returns matching snapshot with specified db name for local-ip hostname', () => { |
There was a problem hiding this comment.
Please add a single blank line between lines 118 and 119.
|
Hi @binokaryg , added the blank line between the it blocks. Please take another look. thanks. |
binokaryg
left a comment
binokaryg
left a comment
There was a problem hiding this comment.
It looks good to merge.
Thank you!
|
🎉 This PR is included in version 6.4.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Description
When using local-ip.medicmobile.org hostnames during local development,
the tool incorrectly treated them as remote hosts, causing the wrong
hash file to be used.
Extended the hostname check in
getHashFileNameto also treat hostnamesending with
.local-ip.medicmobile.orgas local, alongsidelocalhost.Closes #769
Code review items
localhostbehavior is unchanged.License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.