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

tink-cli: Add bash to container image #595

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Mar 10, 2022

Description

Adds bash to tink-cli image

Why is this needed

I've recently been bitten by alpine/busybox's ash not properly implementing
[[ as a builtin while doing a bunch of work on sandbox's scripts. Changing
over to POSIX sh is an option but not really worth it imo as almost everyone
assumes sh == bash already and bash is also nice for interactive use anyway.

Bash ends up adding ~2.2MB to the image which I don't think matters, tink-cli
isn't going to be used in production all that much and its still pretty small.

tink-cli-yes-bash latest 3f59f8e81a34 10 seconds ago 30.4MB
tink-cli-no-bash latest e521b3895111 23 seconds ago 28.2MB

How Has This Been Tested?

Ran container a bunch of different ways.

How are existing users impacted? What migration steps/scripts do we need?

Better tink-cli based scripted stuff?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@mmlb mmlb requested a review from nshalman March 10, 2022 16:17
@mmlb mmlb force-pushed the add-bash-tink-cli-conatiner branch from e93b405 to ab1b234 Compare March 10, 2022 16:19
I've recently been bitten by alpine/busybox's ash not properly implementing
`[[` as a builtin while doing a bunch of work on sandbox's scripts. Changing
over to POSIX sh is an option but not really worth it imo as almost everyone
assumes sh == bash already and bash is also nice for interactive use anyway.

Bash ends up adding ~2.2MB to the image which I don't think matters, tink-cli
isn't going to be used in production all that much and its still pretty small.

tink-cli-yes-bash                latest            3f59f8e81a34   10 seconds ago   30.4MB
tink-cli-no-bash                 latest            e521b3895111   23 seconds ago   28.2MB

Signed-off-by: Manuel Mendez <mmendez@equinix.com>
@mmlb mmlb force-pushed the add-bash-tink-cli-conatiner branch from ab1b234 to efc33b6 Compare March 10, 2022 16:20
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Mar 10, 2022
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #595 (16296cd) into main (2326b77) will not change coverage.
The diff coverage is n/a.

❗ Current head 16296cd differs from pull request most recent head efc33b6. Consider uploading reports for the commit efc33b6 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #595   +/-   ##
=======================================
  Coverage   43.41%   43.41%           
=======================================
  Files          51       52    +1     
  Lines        3107     3107           
=======================================
  Hits         1349     1349           
  Misses       1673     1673           
  Partials       85       85           
Impacted Files Coverage Δ
pkg/apis/core/v1alpha1/hardware_methods.go 100.00% <0.00%> (ø)
pkg/apis/core/v1alpha1/template_types.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2326b77...efc33b6. Read the comment docs.

@mergify mergify bot merged commit e053ada into tinkerbell:main Mar 10, 2022
@mmlb mmlb deleted the add-bash-tink-cli-conatiner branch March 10, 2022 16:36
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants