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

Add compatibility with tink #13

Merged
merged 19 commits into from
Jun 22, 2020
Merged

Add compatibility with tink #13

merged 19 commits into from
Jun 22, 2020

Conversation

kqdeng
Copy link
Contributor

@kqdeng kqdeng commented Jun 2, 2020

(Sorry I made a branch directly off of the main repo...)

Changes:

  • Added an envvar HARDWARE_DATA_MODEL to switch between the old cacher model and the new tinkerbell model
    • if envvar is unset/empty/anything other than tinkerbell, it will default to using the old cacher data model
  • Abstracted exportedHardware among others to take into account new data model

TODO:

  • remove the replace in go.mod pointing to my fork (after the pr in tink gets merged)
  • test Subscribe method (honestly, not sure how)
  • health check tink when creating new tink client?

    hegel/main.go

    Line 145 in ec7c033

    // add health check for tink?

Related PRs:
boots: tinkerbell/smee#27
tink: tinkerbell/tink#64

data_fetch.go Outdated
const (
cacherDataModel = `
{
"id": "8978e7d4-1a55-4845-8a66-a5259236b104",
Copy link

@yetang-equinix yetang-equinix Jun 11, 2020

Choose a reason for hiding this comment

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

why this id is hard coded here? and the same comments go to other hard coded size and numbers in this structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a mock of what would be returned from cacher so that in the tests we don't have to actually make calls to cacher. I'm working on moving these constants into the actual test file so it's not confusing like so.

Choose a reason for hiding this comment

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

if just for testing, fine for now.

yetang-equinix
yetang-equinix previously approved these changes Jun 17, 2020
Copy link

@yetang-equinix yetang-equinix left a comment

Choose a reason for hiding this comment

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

For coding style related comments, like more informational file header and function header comments, we can do that later. But every time we touch a file or a function is an opportunity to add those.

yetang-equinix
yetang-equinix previously approved these changes Jun 22, 2020
@kqdeng kqdeng merged commit 465805d into master Jun 22, 2020
@kqdeng kqdeng deleted the add-tink branch June 22, 2020 18:33
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.

2 participants