Gauravgahlot/new data model #27
Conversation
kqdeng
force-pushed
the
gauravgahlot/new-data-model
branch
2 times, most recently
from
c096453 to
66f0887
Compare
kqdeng
force-pushed
the
gauravgahlot/new-data-model
branch
from
1094d2f to
fb349de
Compare
kqdeng
force-pushed
the
gauravgahlot/new-data-model
branch
from
a085553 to
8ecd0d6
Compare
| func (j *Job) setup(d *packet.Discovery) error { | ||
| mode, netConfig := d.Mode(j.mac), d.NetConfig(j.mac) | ||
| j.Logger = joblog.With("mac", j.mac, "hardware.id", d.Hardware.ID) | ||
| func (j *Job) setup(dp *packet.Discovery) error { |
There was a problem hiding this comment.
It will be nice to have a comments for this function, what will it do? processing dhcp discovery packet? not looks like it .
Same comment goes to all other files and function in tinkerbell suite, too little comment.
| baseURL *url.URL | ||
| consumerToken string | ||
| authToken string | ||
| hardwareClient hardwareGetter | ||
| } | ||
|
|
||
| func NewClient(consumerToken, authToken string, baseURL *url.URL) (*Client, error) { |
There was a problem hiding this comment.
Is this for DHCP client? wondering why we need client functions here in boots
There was a problem hiding this comment.
The hardwareClient refers to the tink/cacher clients responsible for getting the hardware data, see the usage here: https://github.com/tinkerbell/boots/blob/8ecd0d6d08f11d4d6e4bdb9c6b4893815e060ac9/packet/client.go#L46
| @@ -0,0 +1,160 @@ | |||
| package packet | |||
There was a problem hiding this comment.
We have models.go, models_cacher.go, models_tinkerbell.go. As we discussed, it will nice to have file header comments to briefly describe which is for what.
kqdeng
force-pushed
the
gauravgahlot/new-data-model
branch
3 times, most recently
from
f2e2f27 to
a75fa92
Compare
There was a problem hiding this comment.
went through the new changes, can see changes according to our discussion.
For some 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.
Signed-off-by: Gaurav Gahlot <[email protected]>
Signed-off-by: Gaurav Gahlot <[email protected]>
Signed-off-by: Gaurav Gahlot <[email protected]>
kqdeng
force-pushed
the
gauravgahlot/new-data-model
branch
from
a75fa92 to
b88dc4e
Compare
## Description <!--- Please describe what this PR is going to change --> Change from pointers to interface to just an interface for the recent data model split. Took the opportunity to clean up some of the related tests so they would be more correct (when setting env vars) / easier to follow (making TestNewDiscoveryMismatch not panic). ## Why is this needed <!--- Link to issue you have raised --> I think #27 was merged prematurely. Lots of commented out code that should have been deleted, oddness in the pointer-to-interface and some hard to follow tests. This is me polishing some of that up. Fixes: # ## How Has This Been Tested? <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> `go test ./...` ## How are existing users impacted? What migration steps/scripts do we need? <!--- Fixes a bug, unblocks installation, removes a component of the stack etc --> <!--- Requires a DB migration script, etc. --> No anticipated impact. ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [x] added unit or e2e tests - [ ] provided instructions on how to upgrade
Ref: RFC #2 New Hardware data model
Abstracting type Discovery into an interface to settle the differences between the new and old hardware data models.
Changes:
DATA_MODEL_VERSIONto switch between the old cacher model and the new tinkerbell model1, it will default to using the old cacher data modelDiscoveryandHardwareamong others to take into account new data modelTODO:
replaceingo.modpointing to my forkRelated PRs:
hegel: tinkerbell/hegel#13
tink: tinkerbell/tink#64