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

feat: add default entrypoint for the container image #1473

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

onur-ozkan
Copy link
Contributor

I think most developers expect the application to start when running docker run $image instead of terminating the process without doing anything. This PR addresses that expectation.

@onur-ozkan onur-ozkan requested a review from a team as a code owner June 14, 2024 12:47
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@Reecepbcups
Copy link
Member

Reecepbcups commented Jun 14, 2024

I would think developers would expect the relayer binary to run (i.e. help menu) rather than to start. ENTRYPOINT ["rly"] may be more appropriate. Thoughts @onur-ozkan ?

@onur-ozkan
Copy link
Contributor Author

onur-ozkan commented Jun 15, 2024

I am unsure how this will help when we are outside the container. --help(or any command that dumps the command interface) is mostly useful when we are actually using the CLI tool, but in most scenarios when pulling the image you just want to start the application in the container. This is how it's done in other cosmos projects as well like ibc-go and gaia.

@onur-ozkan
Copy link
Contributor Author

I am unsure how this will help when we are outside the container. --help(or any command that dumps the command interface) is mostly useful when we are actually using the CLI tool, but in most scenarios you just want to start the application in the container. This is how it's done in other cosmos projects as well like ibc-go and gaia.

Oh, seems like ibc-go image does the exact thing you said above.

@Reecepbcups
Copy link
Member

Reecepbcups commented Jun 15, 2024

@onur-ozkan Yea i think just rly is the best approach. Then adding on arguments as you need them (query, tx, start, etc).

I am going to leave this for someone else to decide on the best approach as this is a breaking change. Thanks for the contribution here! Great thing for us to discuss & add ideally

@Reecepbcups Reecepbcups changed the title add default entrypoint for the container image feat!: add default entrypoint for the container image Jun 15, 2024
@onur-ozkan
Copy link
Contributor Author

this is a breaking change.

Could you please clarify this a bit ? 🙂 At the moment there is no default entry point specified, so people have to specify one manually to make this container to do something useful. By doing so, the entry point we are adding with this PR will be overridden which means they won't notice any difference.

@Reecepbcups Reecepbcups changed the title feat!: add default entrypoint for the container image feat: add default entrypoint for the container image Jun 17, 2024
@Reecepbcups
Copy link
Member

Sorry, I was thinking docker CMD and not Entrypoint. Entrypoint is not a breaking change. Updated title and marketed out my comment. Thanks!

Copy link
Member

@Reecepbcups Reecepbcups 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 just going to take action here and approve it as is. thanks! sorry for the wait here

@Reecepbcups Reecepbcups enabled auto-merge (squash) July 16, 2024 12:36
@Reecepbcups
Copy link
Member

Ci failure is due to rate limiting, will re-run this later

@Reecepbcups Reecepbcups merged commit c5dd657 into cosmos:main Jul 24, 2024
21 checks passed
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.

2 participants