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

update docker setup script #223

Closed
wants to merge 1 commit into from
Closed

update docker setup script #223

wants to merge 1 commit into from

Conversation

agnes512
Copy link
Contributor

Description

connect to loopbackio/loopback-next#3968

The current script failed to create new database:

curl --request PUT --url http://$USER:$PASSWORD@$HOST:$PORT/$DATABASE > /dev/null 2>&1

Update the script so that we can run acceptance tests in repository-tests on cloudant.

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@agnes512 agnes512 requested a review from virkt25 as a code owner October 18, 2019 01:46
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I am not familiar with Cloudant and cannot really asses impact of these changes. I am fine to land them as long as they work for you.

Please get at least one more approval, preferably from somebody more familiar with Cloudant.

@@ -105,7 +105,7 @@ printf "\n${CYAN}Cloudant started.${PLAIN}\n"

## create database
printf "\n${RED}>> Creating database in Cloudant${PLAIN}"
curl --request PUT --url http://$USER:$PASSWORD@$HOST:$PORT/$DATABASE > /dev/null 2>&1
curl -i -X PUT http://$USER:$PASSWORD@$HOST:$PORT/$DATABASE > /dev/null 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove 2>&1, so that when the request fails, the errors are printed by the setup.sh script?

I also believe that -i is not necessary, since you are discarding the output to /dev/null anyways.

Quoting from man curl:

-i, --include
Include the HTTP-header in the output. The HTTP-header includes things like server-name, date of the document, HTTP-version and more...

See also -v, --verbose.

Proposed line:

Suggested change
curl -i -X PUT http://$USER:$PASSWORD@$HOST:$PORT/$DATABASE > /dev/null 2>&1
curl -X PUT http://$USER:$PASSWORD@$HOST:$PORT/$DATABASE > /dev/null

Copy link
Contributor

Choose a reason for hiding this comment

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

npm run mocha
npm run test

fail with some errors on my laptop, but works in CI build?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm some tests fail on laptop with master branch too.

Why docker-compose use the :latest tag, but test.js use :2.0.1 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

seem to be the same

image

Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

We need to find out why

@@ -105,7 +105,7 @@ printf "\n${CYAN}Cloudant started.${PLAIN}\n"

## create database
printf "\n${RED}>> Creating database in Cloudant${PLAIN}"
curl --request PUT --url http://$USER:$PASSWORD@$HOST:$PORT/$DATABASE > /dev/null 2>&1
curl -i -X PUT http://$USER:$PASSWORD@$HOST:$PORT/$DATABASE > /dev/null 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

npm run mocha
npm run test

fail with some errors on my laptop, but works in CI build?

@@ -1,18 +1,17 @@
version: '2'
version: "3"
services:
cloudant:
image: ibmcom/cloudant-developer:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

This image is retired, I would suggest we use ibmcom/couchdb3 instead, see https://hub.docker.com/r/ibmcom/couchdb3.

@jannyHou jannyHou mentioned this pull request Oct 18, 2019
2 tasks
@agnes512
Copy link
Contributor Author

We need to find out why

It failed before the changes.

@agnes512
Copy link
Contributor Author

closing as we've decided to use couchdb to run the test.

@agnes512 agnes512 closed this Oct 21, 2019
@emonddr
Copy link
Contributor

emonddr commented Oct 24, 2019

@agnes512 , can you delete this branch now? Since this PR is not going to merge? thx.

@agnes512 agnes512 deleted the update-setup-script branch October 24, 2019 13:36
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