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

Allow entrypoints by default #312

Merged
merged 5 commits into from
Oct 9, 2023
Merged

Allow entrypoints by default #312

merged 5 commits into from
Oct 9, 2023

Conversation

bossmc
Copy link
Collaborator

@bossmc bossmc commented Jul 20, 2023

Why this change?

Following the principle of least surprise, don't suppress image entrypoints unless asked to.

Relevant testing

Tested with an image that runs an entrypoint before starting the image.

Checks

These aren't hard requirements, just guidelines

  • New/modified Rust code formatted with cargo fmt
  • Documentation and README.md updated for this change, if necessary
  • CHANGELOG.md updated for this change

@bossmc bossmc force-pushed the allow-entrypoints-by-default branch 2 times, most recently from 1dcc25d to 8c58fea Compare July 20, 2023 16:28
@maxdymond
Copy link
Collaborator

LGTM but would like a review from Richard as well.

Copy link
Collaborator

@rlupton20 rlupton20 left a comment

Choose a reason for hiding this comment

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

Fine by me. I'm not 100% what is the least surprise to people - I think most users, for whatever reason, think floki is some magic thing which always gives you an interactive session in any docker container (hence entrypoint suppression, and shell as the default entrypoint - to the first approximation the thing most likely to achieve this).

Equally, most containers don't have weird entrypoints that need supressing (especially the ones floki is used with). For those that do, having the override is important. It's probably more reasonable that most containers behave as they normally do, so entrypoint override by default I suppose is a default created by the rare case, so more suprising. I don't know.

Anyway, happy with the change - one small thing on the default shell (which to be honest was probably as you've written for ages already anyway).

src/config.rs Outdated Show resolved Hide resolved
Signed-off-by: Andy Caldwell <andycaldwell@microsoft.com>
@maxdymond
Copy link
Collaborator

I've deprecated the actions-rs Github Actions as they are not being actively maintained - you might need to tweak your MR so that you're using the new code.

Signed-off-by: Andy Caldwell <andycaldwell@microsoft.com>
Signed-off-by: Andy Caldwell <andycaldwell@microsoft.com>
Signed-off-by: Andy Caldwell <andycaldwell@microsoft.com>
@bossmc bossmc force-pushed the allow-entrypoints-by-default branch from ed6e431 to 8e3a2e3 Compare October 9, 2023 16:08
@bossmc bossmc merged commit 5cb07c8 into main Oct 9, 2023
9 checks passed
@bossmc bossmc deleted the allow-entrypoints-by-default branch October 9, 2023 16:27
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.

None yet

3 participants