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

Some fixes to allow running echo sql app in one go #102

Closed
wants to merge 3 commits into from

Conversation

hp77-creator
Copy link

Changes:

  1. Changed network name to keploy-network since that is what keploy creates when we install keploy
  2. Changed host name to postgres from postgresDB because docker uses service name

There will be some changes that will need to be added in docs as well to ensure that user is able to run this app in one-go

Some suggestions like adding --rm command in keploy record ... command so that user doesn't see the error that container is still up/or change the app name.
Open to suggestions. For now this also seemed good so sending a PR for the same.

Signed-off-by: Himanshu Pandey <himanshu.dn.pandey@gmail.com>
Signed-off-by: Himanshu Pandey <himanshu.dn.pandey@gmail.com>
Signed-off-by: Himanshu Pandey <himanshu.dn.pandey@gmail.com>
@Sarthak160
Copy link
Member

Hey @hp77-creator, Since user can use its custom network as well , keploy will automatically use that network instead of using keploy-network specifically . In the alias as well user is free to change the network name , so here keploy-network seems to be good for simplicity . But reason to use some random network name was to showcase automatic custom network interception feature of keploy.

@Sarthak160
Copy link
Member

So i think, in this you can rename this to custom-network instead of baba and add in comments that if user doesnt provide any network by default .

@hp77-creator
Copy link
Author

But reason to use some random network name was to showcase automatic custom network interception feature of keploy.

I see but on the docs that I was following, keploy record was passing --network flag as keploy-network, Either one of the two should be changed imo. I felt changing network is better, given docker might change name of the container and based on the network if more containers are spinned up.

So i think, in this you can rename this to custom-network instead of baba and add in comments that if user doesnt provide any network by default .

I didn't get this. Do you want me to rename keploy-network to custom-network? Doing that will work given that we are expecting a new user to run following command: keploy record -c "docker run -p 8082:8082 --name echoSqlApp --network keploy-network echo-app:1.0"

@Sarthak160
Copy link
Member

Sarthak160 commented Feb 27, 2024

But reason to use some random network name was to showcase automatic custom network interception feature of keploy.

I see but on the docs that I was following, keploy record was passing --network flag as keploy-network, Either one of the two should be changed imo. I felt changing network is better, given docker might change name of the container and based on the network if more containers are spinned up.

So i think, in this you can rename this to custom-network instead of baba and add in comments that if user doesnt provide any network by default .

I didn't get this. Do you want me to rename keploy-network to custom-network? Doing that will work given that we are expecting a new user to run following command: keploy record -c "docker run -p 8082:8082 --name echoSqlApp --network keploy-network echo-app:1.0"

yea, that's true that if container name changes we have to change the host name but since service name is mapped we can consider using that. And at the docs it has been mention to run with docker run command. But above I was talking about docker-compose file only in case of docker compose even if you don't provide any network it will still work. Because keploy creates a kdocker-compose file where it automatically creates a keploy-network for that user container.

@Sonichigo Sonichigo closed this Jul 26, 2024
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.

3 participants