Develop policy to get Redfish maintainers involved when appropriate

[paulfertser]

As discussed at https://gerrit.openbmc.org/c/openbmc/webui-vue/+/70758/2#message-57c7126ba7a8b3051042cee18201d6c830de717d sometimes webui-vue would really benefit from talking to people intimately familiar with Redfish. Without their feedback wrong solutions get incorporated in the codebase and lead to problems sometimes years later.

I propose to document and enforce during review some operational measures which would help in situations like that.

An obvious idea would be to get Redfish maintainers in the loop each and every time a new Redfish endpoint is introduced into the codebase. That would allow them to validate that API is used as intended.

Open questtions:

1. Are there any other straightforward "triggers" that can be added?
2. What is the list of Redfish maintainers willing to participate in reviews like that?

[edtanous]

Can you just propose this in the webui-vue repo in a gerrit review? github bugs aren't really a great way for reviewing policy.

An obvious idea would be to get Redfish maintainers in the loop each and every time a new Redfish endpoint is introduced into the codebase.

This isn't really workable as written. The whole point of having a standard is that both sides can code to this, and it will be correct. While i'm very open to reviewing webui code, I'm not going to review every patch, and Gunnar (the other redfish maintainer) is already a webui maintainer, so I'm not sure what this accomplishes.

Instead, how about writing something like:

webui-vue shall code to Redfish as a protocol standard, and in places where that is not possible, new commits shall:
A. Document the deviation from the standard in "Redfish_deviations.md", along with an engineering justification on why using the standard wasn't possible, after the limitations in the standard were brought to DMTF.
B. Limitations in bmcweb should be brought to the OpenBMC mailing list, and proposed as gerrit reviews implementing the missing functionality.
C.  If server-side Redfish functionality appears to be implemented incorrectly to the Redfish standard in OpenBMC, it will be brought to the attention of the maintainers through the OpenBMC mailing list.

[paulfertser]

Can you just propose this in the webui-vue repo in a gerrit review? github bugs aren't really a great way for reviewing policy.

OK, I will since that's desired.

An obvious idea would be to get Redfish maintainers in the loop each and every time a new Redfish endpoint is introduced into the codebase.

This isn't really workable as written. The whole point of having a standard is that both sides can code to this, and it will be correct.

Indeed, but there's also Hyrum's law: "With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody." https://en.wikipedia.org/wiki/API

While i'm very open to reviewing webui code, I'm not going to review every patch,

My proposal was for webui-vue maintainers to draw special attention only to patches introducing access to /new/ (for the webui-vue codebase) endpoints (and probably to new fields when in doubt).

Gunnar (the other redfish maintainer) is already a webui maintainer, so I'm not sure what this accomplishes.

The patch in question, https://gerrit.openbmc.org/c/openbmc/webui-vue/+/58883 , was in review for 4 months, and Gunnar didn't notice it doing wrong. There must be a way to fulfill your "I really wish people would loop in Redfish maintainers when they hit walls like this, instead of implementing things like getting a ManagerAccount that might not exist, using bmcweb-specific implementation details" for cases like that, I just do not see it quite yet.

My idea was to somehow raise awareness for the most active webui-vue maintainers and provide them with an instrument to tell apart regular improvements from patches that really abuse the API.

Since I did not yet have a clear idea of how to make that possible (and if it's at all needed) I created a ticket to discuss rather than a patch.

Instead, how about writing something like:

webui-vue shall code to Redfish as a protocol standard, and in places where that is not possible, new commits shall:
A. Document the deviation from the standard in "Redfish_deviations.md", along with an engineering justification on why using the standard wasn't possible, after the limitations in the standard were brought to DMTF.
B. Limitations in bmcweb should be brought to the OpenBMC mailing list, and proposed as gerrit reviews implementing the missing functionality.
C.  If server-side Redfish functionality appears to be implemented incorrectly to the Redfish standard in OpenBMC, it will be brought to the attention of the maintainers through the OpenBMC mailing list.

I guess that would already be an improvement, especially if enforced by the maintainers.

[edtanous]

You quote Hyrums law, but forgot to add the "therefore do X". Here's my version:

#43 (comment)

But I've never used the webui enough to need it, or have a strong opinion on getting it implemented, but essentially it involves make an API that makes it difficult to do the wrong thing, and easy to do the right thing. Expecting more review on every commit and writing it down works in the short term. Making an API that's impossible to use incorrectly is better. I review 110% of what I'm able to, so does Gunnar, you'll note that neither of us were on the patch in question above, which is fine, but we need a way that folks can implement things correctly and get it right before review. Getting some form of policy written down is the first step, which could've immediately been quoted for blocks of code like:

webui-vue/src/router/routes.js

Line 35 in b325541

const roles = {

Where we're essentially hardcoding what's already in the RoleCollection, and tying us further to bmcweb.

[paulfertser]

Getting some form of policy written down is the first step, which could've immediately been quoted for blocks of code like:

webui-vue/src/router/routes.js

Line 35 in b325541

const roles = {

Where we're essentially hardcoding what's already in the RoleCollection, and tying us further to bmcweb.

As a side note, that commit added this constant associative array not once, but four (!) times, identical wrong thing in four different files:
https://gerrit.openbmc.org/c/openbmc/webui-vue/+/58883/13/src/components/AppNavigation/AppNavigationMixin.js#9
https://gerrit.openbmc.org/c/openbmc/webui-vue/+/58883/13/src/env/components/AppNavigation/intel.js#10
https://gerrit.openbmc.org/c/openbmc/webui-vue/+/58883/13/src/env/router/intel.js#30
https://gerrit.openbmc.org/c/openbmc/webui-vue/+/58883/13/src/router/routes.js#34

As to your version of a suggestion that actually demonstrates the way forward, it could have been very nice if only the parties involved actually valued input like that. But to quote from the next message "Intel is going to be working on that .. we agreed to not add any more hardcoded endpoints". The bug was open in Oct 2020, your sample code to show feasibiliity of implementation posted on Mar 2021, it's May 2024 now and the bug is still open. Given this timeline it might seem that your other idea ("I would happy to help make this a higher priority for the maintainers by obfuscating the bmcweb URI identifiers") could have worked much bettter...

[edtanous]

you'll note that to that end, I got this patch merged, so we can start doing that.
openbmc/bmcweb@253f11b

As a side note, that commit added this constant associative array not once, but four (!) times, identical wrong thing in four different files:

Sounds like a good patch you could work on to make that better in code.