Skip to content

Conversation

@HirokiUchikawa
Copy link
Member

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

Currently test cases for petstore use http://petstore.swagger.io .
I changed it to use petstore on docker on travis.
and I removed node.js v7 from travis because node.js v7 is not LTS.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@kazuhitoyokoi
Copy link
Member

kazuhitoyokoi commented Jan 22, 2018

@HirokiUchikawa
Thank you for the pull request. The replacement will be useful to test various version of Node.js simultaneously because swagger petstore server has status.

I checked docker version of petstore server. The value of "host" in swagger.json is "localhost:8002".
Therefore, I think that the followings are correct settings.

(1) hosts configuration is not required.

-  hosts:
 -    - petstore.swagger.io

(2) SWAGGER_HOST value in docker run command is not required.
(3) Port number of docker instance is "8002".

-  - docker run -d -e SWAGGER_HOST=http://petstore.swagger.io -e SWAGGER_BASE_PATH=/v2 -p 80:8080 swaggerapi/petstore
+  - docker run -d -e SWAGGER_BASE_PATH=/v2 -p 8002:8080 swaggerapi/petstore

Could you check the settings?

@HirokiUchikawa
Copy link
Member Author

@kazuhitoyokoi
Thank you for the feedback.
I will check the settings and fix my pull request.

@HirokiUchikawa
Copy link
Member Author

@kazuhitoyokoi
I investigated this problem and found the cause.

This website says that the host is set by SWAGGER_HOST.
But Actually the host is set by SWAGGER_URL in Docker.
So I will replace SWAGGER_HOST with SWAGGER_URL to fix this problem.

I think, if your suggestion is applied,the developer will have to make two patterns test case for http://petstore.swagger.io and http://localhost:8002.
I think it is better to handle only one url.

Let me know if you have any questions.

@kazuhitoyokoi
Copy link
Member

@HirokiUchikawa Good. 💯 Thank you for your investigation. Please ignore my suggestion and use environment variable, SWAGGER_URL instead of SWAGGER_HOST.

@coveralls
Copy link

coveralls commented Jan 23, 2018

Coverage Status

Coverage remained the same at 32.893% when pulling c8e197f on HirokiUchikawa:master-travis into b669a02 on node-red:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 32.893% when pulling c8e197f on HirokiUchikawa:master-travis into b669a02 on node-red:master.

@HiroyasuNishiyama HiroyasuNishiyama merged commit 1000935 into node-red:master Jan 23, 2018
@HirokiUchikawa HirokiUchikawa deleted the master-travis branch January 23, 2018 06:14
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.

4 participants