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

Support event subscription and submitTestEvent for various vendors #187

Merged
merged 1 commit into from
Jul 9, 2022

Conversation

nwaizer
Copy link
Contributor

@nwaizer nwaizer commented Jul 6, 2022

No description provided.

@nwaizer nwaizer changed the title Events4vendors Support event subscription and submitTestEvent for various vendors Jul 6, 2022
Copy link
Owner

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Imports still need to be cleaned up. Make sure to run make or make lint locally.

Looks like some local files were accidentally included.

References to oem code still need to be removed from the core.

@nwaizer nwaizer force-pushed the events4vendors branch 2 times, most recently from 2431ad0 to bf88f9e Compare July 6, 2022 20:34
@nwaizer
Copy link
Contributor Author

nwaizer commented Jul 6, 2022

Hi Sean,
Thanks again for your help on this.
0. lint now shows only this:

oem/hpe/hpe.go:8: File is not `goimports`-ed with -local github.com/stmcginnis (goimports)
	"encoding/json"
redfish/eventdestination.go:10: File is not `goimports`-ed with -local github.com/stmcginnis (goimports)
	oemZt "github.com/stmcginnis/gofish/oem/zt"
redfish/eventservice.go:9: File is not `goimports`-ed with -local github.com/stmcginnis (goimports)
	"fmt"

I tried to run goimports -w on these files, to no avail. what to do?

  1. Extra files are used in testing the new function as SubscribeZT() and SendEventDell()
  2. So you do not want to use a common CreateEventSubscription() to work for all three oem vendors?
  3. Same for not using a common SubmitTestEvent() for all three vendors?

oem/dell/dell.go Outdated Show resolved Hide resolved
oem/dell/dell.go Outdated Show resolved Hide resolved
oem/dell/dell.go Outdated Show resolved Hide resolved
oem/dell/test/dell_test.go Outdated Show resolved Hide resolved
oem/dell/test/dell_test.go Outdated Show resolved Hide resolved
oem/zt/zt.go Outdated Show resolved Hide resolved
redfish/eventdestination.go Outdated Show resolved Hide resolved
redfish/eventdestination.go Outdated Show resolved Hide resolved
redfish/eventservice.go Outdated Show resolved Hide resolved
redfish/eventservice.go Outdated Show resolved Hide resolved
@nwaizer nwaizer force-pushed the events4vendors branch 2 times, most recently from 4d96540 to 8a200b8 Compare July 7, 2022 19:08
@nwaizer
Copy link
Contributor Author

nwaizer commented Jul 7, 2022

all done and ready to merge?

.gitignore Outdated
@@ -15,3 +15,4 @@ bin/*
**/gofiles/*
**/*.zip
**/DSP*
.idea/
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this line. See header at top of file.

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

oem/zt/zt.go Outdated
ID int `json:"ID"`
Name string `json:"Name"`
Protocol string `json:"Protocol"`
common.Status `json:"Status"`
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't look right. Defined this way, the properties of the Status struct are direct properties of the response type. But according to the JSON in the test code, there should be a Status property of type common.Status.

So current definition is:

{
    "Health": "OK",
    "HealthRollup": "OK",
    "State": "Enabled"
}

But the example from the test JSON is:

{
    "Status": {
        "Health": "OK",
        "HealthRollup": "OK",
        "State": "Enabled"
    }
}

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

oem/zt/zt.go Outdated
}

// SubmitTestEvent sends event according to msgId and returns error.
func SubmitTestEvent(client common.Client, msgID string) error {
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, looking closer here and notice this isn't quite right yet either. Like the example given earlier, these calls need to be done on OEM versions of the standard objects. So you would get an EventService instance, then to access the OEM specific implementations for that you would need to get the right version. Something like:

eventService, _ := x.EventService()
ztEventService, _ := zt.FromEventService(eventService)
ztEventService.SubmitTestEvent("my message")

So there still needs to be a ZTEventService defined, then this would be a call on an instance of that object.

https://github.com/stmcginnis/gofish/blob/main/redfish/eventservice.go#L334

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its ok now. please provide feedback if not

Copy link
Owner

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Still a few issues, but I will clean up with a follow up. Thanks for working through all of this!

@stmcginnis stmcginnis merged commit d3e2dde into stmcginnis:main Jul 9, 2022
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