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.

Sign up for GitHub

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

Jump to bottom

Hotfix - use DeprecatedWatch for now #52

Merged
merged 4 commits into from
Dec 23, 2020
Merged

Hotfix - use DeprecatedWatch for now #52

merged 4 commits into from
Dec 23, 2020

Conversation

gauravgahlot
Copy link
Contributor

Description

The PR fixes build issues in Hegel due to recent changes in Tink wrt to event-driven design.
For now, Hegel will continue using the old watch style with DeprecatedWatch.
Changes wrt to the new watch stream will be done in a separate PR.

Why is this needed

The PR fixes build issues in Hegel due to recent changes in Tink wrt to event-driven design.

@gauravgahlot gauravgahlot requested a review from kqdeng December 8, 2020 06:54
@gauravgahlot gauravgahlot self-assigned this Dec 8, 2020
kqdeng
kqdeng suggested changes Dec 8, 2020
View reviewed changes
grpc_server_test.go
@@ -139,8 +139,8 @@ func TestGetByIPTinkerbell(t *testing.T) {
t.Fatalf("unexpected filesystem mount format, want: %v, got: %v\n", test.filesystemFormat, hw.Metadata.Instance.Storage.Filesystems[0].Mount.Format)
}
}
if hw.Metadata.Instance.OperatingSystemVersion.OsSlug != test.osSlug {
t.Fatalf("unexpected os slug, want: %v, got: %v\n", test.osSlug, hw.Metadata.Instance.OperatingSystemVersion.OsSlug)
if hw.Metadata.Instance.OperatingSystem.Slug != test.osSlug {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's the hw.Metadata.Instance that is causing the nil pointer dereference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kdeng3849
I made the following corrections in metadata JSON:

operating_system_version   > operating_system
os_slug   > slug

Tests pass but I'm not certain if it is doing what's expected.

At the matster branch when I debug the test hw.Metadata fields are NIL. If we print the ehw this is what we have and it doesn't contain metadata. Is it expected?

{"id":"363115b0-f03d-4ce5-9a15-5514193d131a","network":{"interfaces":[{"dhcp":{"arch":"x86_64","hostname":"server001","ip":{"address":"192.168.1.5","gateway":"192.168.1.1","netmask":"255.255.255.248"},"lease_time":86400,"mac":"ec:0d:9a:c0:01:0c"},"netboot":{"allow_pxe":true,"allow_workflow":true,"ipxe":{"contents":"#!ipxe","url":"http://url/menu.ipxe"},"osie":{"kernel":"vmlinuz-x86_64"}}}]}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out it was returning on the first test case with empty metadata when it should really be continue. Can you change that for me (also update the comment)? Thanks

hegel/grpc_server_test.go

Line 113 in 43d76c5

if reflect.DeepEqual(&hw.Metadata, &packet.Metadata{}) { // return if metadata is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. 👍🏼

@codecov
Copy link

codecov bot commented Dec 9, 2020 •
edited
Loading

Codecov Report

Merging #52 (cd8141b) into master (f5b5032) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #52   +/-   ##
=======================================
  Coverage   24.40%   24.40%           
=======================================
  Files           5        5           
  Lines         504      504           
=======================================
  Hits          123      123           
  Misses        359      359           
  Partials       22       22
Impacted Files Coverage Δ
grpc_server.go 22.53% <0.00%> (ø)
main.go 9.00% <0.00%> (ø)
mock.go 72.22% <ø> (ø)

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 f5b5032...cd8141b. Read the comment docs.

@gauravgahlot gauravgahlot requested a review from kqdeng December 10, 2020 03:50
@gauravgahlot gauravgahlot added the ready-to-merge Signal to Mergify to merge the PR. label Dec 10, 2020
@gauravgahlot
go mod tidy
6ee70c0 
Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot
use DeprecatedWatch for now
b33c97b 
Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot
fix test
a7a0664 
Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot
metadata correction
cd8141b 
Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot gauravgahlot requested a review from Cbkhare December 23, 2020 11:57
@Cbkhare Cbkhare self-requested a review December 23, 2020 12:27
@gauravgahlot gauravgahlot removed the request for review from kqdeng December 23, 2020 12:28
@gauravgahlot gauravgahlot merged commit e8f08dd into tinkerbell:master Dec 23, 2020
@gauravgahlot gauravgahlot deleted the hegel-fix branch December 23, 2020 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kqdeng kqdeng kqdeng requested changes

@Cbkhare Cbkhare Cbkhare approved these changes

Assignees

@gauravgahlot gauravgahlot

Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gauravgahlot @Cbkhare @kqdeng