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

Fix tftp ReadFile, Add tests: #134

Conversation

jacobweinstock
Copy link
Member

Description

tftp ReadFile was missing a return if AllowPxe was false.
This adds the return and adds tests for the AllowPxe
gate that was just recently added.
this library: https://github.com/bouk/monkey, was used in the unit tests and requires running tests with -gcflags=-l.
the github actions were updated to accommodate this.
a few testing helpers were added to the makefile.

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

tftp ReadFile was missing a return if AllowPxe was false.
This adds the return and adds tests for the AllowPxe
gate that was just recently added.

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #134 (479adf0) into master (56f401c) will increase coverage by 1.86%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   40.65%   42.52%   +1.86%     
==========================================
  Files          40       40              
  Lines        2086     2086              
==========================================
+ Hits          848      887      +39     
+ Misses       1152     1113      -39     
  Partials       86       86              
Impacted Files Coverage Δ
tftp.go 35.82% <100.00%> (+34.32%) ⬆️
http.go 33.13% <0.00%> (+9.46%) ⬆️

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 56f401c...e75bb4f. Read the comment docs.

@jacobweinstock jacobweinstock added the kind/bug Categorizes issue or PR as related to a bug. label Mar 11, 2021
@gianarb
Copy link
Contributor

gianarb commented Mar 12, 2021

I don't know what that library is used for, but it is archived... We should figure out an alternative I suppose at some point


for name, tc := range tests {
t.Run(name, func(t *testing.T) {
monkey.Patch(job.CreateFromRemoteAddr, func(_ string) (job.Job, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤯

@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Mar 12, 2021
@mmlb mmlb removed the request for review from markjacksonfishing March 12, 2021 15:03
@jacobweinstock jacobweinstock merged commit b8320d5 into tinkerbell:master Mar 12, 2021
@jacobweinstock jacobweinstock deleted the pxe-based-on-hardware-allow-pxe branch March 12, 2021 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. 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