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.

Sign up for GitHub

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

Jump to bottom

fix: replace hostname with host in admin config #876

Merged
merged 4 commits into from
Mar 13, 2019
Merged

fix: replace hostname with host in admin config #876

merged 4 commits into from
Mar 13, 2019

Conversation

ravikp7
Copy link
Contributor

@ravikp7 ravikp7 commented Mar 11, 2019

  • improves consistency with apiEndpoints config
  • generators now use host in admin config
  • hostname is still supported, but warning is shown

fixes #432

@codecov
Copy link

codecov bot commented Mar 11, 2019 •
edited
Loading

Codecov Report

Merging #876 into master will decrease coverage by 0.1%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #876      +/-   ##
==========================================
- Coverage   89.25%   89.15%   -0.11%     
==========================================
  Files         135      135              
  Lines        3649     3652       +3     
==========================================
- Hits         3257     3256       -1     
- Misses        392      396       +4
Impacted Files Coverage Δ
bin/eg-generator.js 87.17% <0%> (ø) ⬆️
lib/rest/index.js 91.11% <80%> (-1.75%) ⬇️
lib/config/config.js 90.21% <0%> (-3.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5942b0...2345555. Read the comment docs.

XVincentX
XVincentX requested changes Mar 12, 2019
View reviewed changes
Copy link
Member

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

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

Nice start! This is going to require some work though and I'd appreciate if you could put at least one test, although I can live without this.

bin/eg-generator.js Outdated Show resolved Hide resolved
lib/config/schemas/gateway.config.json Show resolved Hide resolved
lib/rest/index.js Outdated Show resolved Hide resolved
@ravikp7
Copy link
Contributor Author

ravikp7 commented Mar 12, 2019

I'm not sure of where to put tests or what to test for the changes made in this PR.
Can you please guide me on this?

@XVincentX
Copy link
Member

I'm not sure of where to put tests or what to test for the changes made in this PR.

Good point. Let's first make sure the code is ready and then CodeCov will guide us in the part that needs to be covered.

It's not exactly that mandatory to test this stuff (I can live with it) but it'd be great to onboard you appropriately :)

Keep up the good work!

@ravikp7 ravikp7 force-pushed the fix-hostname-consistency branch 3 times, most recently from 116ccc4 to bdf2ef7 Compare March 12, 2019 22:19
@XVincentX XVincentX force-pushed the fix-hostname-consistency branch 3 times, most recently from 9bba851 to cfc81da Compare March 13, 2019 15:47
@ravikp7 @XVincentX
fix: replace hostname with host in admin config
69185dd 
- improves consistency with apiEndpoints config
- generators now use host in admin config
- hostname is still supported, but warning is shown

fixes ExpressGateway#432
@XVincentX XVincentX force-pushed the fix-hostname-consistency branch 3 times, most recently from 5097501 to 365c1e2 Compare March 13, 2019 15:55
@XVincentX XVincentX force-pushed the fix-hostname-consistency branch from 365c1e2 to 5c19ba2 Compare March 13, 2019 15:56
@XVincentX XVincentX force-pushed the fix-hostname-consistency branch from 513e77a to 2345555 Compare March 13, 2019 16:10
@XVincentX
Copy link
Member

I also went ahead and checked what's up with the tests and the untested part do not concern this PR. Good to go! 🚀

@XVincentX XVincentX merged commit 8cb9ab9 into ExpressGateway:master Mar 13, 2019
@ravikp7 ravikp7 deleted the fix-hostname-consistency branch March 13, 2019 17:29
@snyk-bot snyk-bot mentioned this pull request Jan 24, 2020
Open
gatherchou pushed a commit to yilu-tech/express-gateway that referenced this pull request Jul 29, 2021
@XVincentX
Merge pull request ExpressGateway#876 from ravikp7/fix-hostname-consi…
2fc2dec 
…stency

fix: replace hostname with host in admin config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XVincentX XVincentX XVincentX approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

host versus hostname: need consistency
2 participants
@ravikp7 @XVincentX