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 getWorkerID when mac addresses have captalized letters #486

Merged
merged 3 commits into from
Jul 5, 2021

Conversation

moadqassem
Copy link
Member

Description

Why is this needed

To solve the worker not found errors, when the mac address of the worker is in upper case.

Fixes: #485

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

@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #486 (07b5572) into master (4bdd9cb) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #486   +/-   ##
=======================================
  Coverage   32.70%   32.70%           
=======================================
  Files          44       44           
  Lines        3137     3137           
=======================================
  Hits         1026     1026           
  Misses       2019     2019           
  Partials       92       92           
Impacted Files Coverage Δ
db/workflow.go 30.63% <100.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 4bdd9cb...07b5572. Read the comment docs.

@tstromberg
Copy link
Contributor

This PR looks great - is there any way we could have a unit test added to ensure that we don't break this again in the future?

@moadqassem
Copy link
Member Author

moadqassem commented Jun 22, 2021

@tstromberg you can extend this case here https://github.com/tinkerbell/tink/blob/master/db/workflow_test.go#L42-L68 and then do a fetch request or workflow get request with a device id that has the characters in lower case.

@gauravgahlot
Copy link
Contributor

Hey @moadqassem, do you mind updating the branch and extend the test as well? It would be great to merge the PR then.

@moadqassem
Copy link
Member Author

@gauravgahlot I have added a test case to cover that d502ede

cc @tstromberg

@gauravgahlot gauravgahlot requested review from gauravgahlot and removed request for parauliya July 5, 2021 05:58
@gauravgahlot
Copy link
Contributor

Thank you @moadqassem 🚀

@gauravgahlot gauravgahlot merged commit 8ea8a0e into tinkerbell:master Jul 5, 2021
@moadqassem moadqassem deleted the fix-get-worker-by-id branch July 5, 2021 08:32
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.

tink-server fails to compare mac addresses
3 participants