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 registry, registry username and password to be empty strings #254

Merged
merged 2 commits into from
Apr 18, 2022

Conversation

jacobweinstock
Copy link
Member

@jacobweinstock jacobweinstock commented Apr 17, 2022

Description

Allow registry, registry username, and password to be empty strings. These values are passed as kernel command line args for OSIE's boot up and then used by Tink worker when it pulls and runs containers. With this change to Tink worker, allowing the empty string here provides Tink worker the ability to specify any fully qualified container image in a template. e.g. docker.io/alpine, quay.io/tinkerbell-actions/cexec, public.ecr.aws/l0g8r8j6/tinkerbell/pbnj, etc

This change is backward compatible and shouldn't negatively affect using the Cacher backend.

Why is this needed

Fixes: #

How Has This Been Tested?

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

Checklist:

I have:

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

These values are passed as kernel command line args for OSIE's boot up
and then used by Tink worker when it pulls and runs containers. With a
recent change Tink worker, allowing the empty string here provides Tink
worker the ability to specify any full qualified container image in
a template.

This change is backward compatible and shouldn't negatively affect
using the Cacher backend.

Signed-off-by: Jacob Weinstock <[email protected]>
@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #254 (3a3f97b) into main (de2d781) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
- Coverage   39.57%   39.53%   -0.05%     
==========================================
  Files          39       39              
  Lines        2772     2775       +3     
==========================================
  Hits         1097     1097              
- Misses       1587     1590       +3     
  Partials       88       88              
Impacted Files Coverage Δ
installers/osie/main.go 64.51% <0.00%> (-1.60%) ⬇️
installers/osie/mirror.go 24.13% <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 de2d781...3a3f97b. Read the comment docs.

@jacobweinstock jacobweinstock requested review from mmlb and nshalman April 18, 2022 21:57
Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

The changes lgtm. I'm wondering we don't go ahead and make it so the kernel args aren't populated if the strings are empty?

@jacobweinstock
Copy link
Member Author

The changes lgtm. I'm wondering we don't go ahead and make it so the kernel args aren't populated if the strings are empty?

sure, i can add that.

@jacobweinstock jacobweinstock requested a review from mmlb April 18, 2022 22:16
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Apr 18, 2022
@jacobweinstock jacobweinstock removed the request for review from nshalman April 18, 2022 22:39
@mergify mergify bot merged commit 1efcbab into tinkerbell:main Apr 18, 2022
@jacobweinstock jacobweinstock deleted the make-registry-optional branch April 18, 2022 22:40
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.

2 participants