docs(parameters): add DevOps Dojo: ROS Node Config documentation

[kaspermeck-arm]

Updated the guidelines for how to configure the ROS node parameters.

Description

Updating coding guidelines for the ROS node parameters according to the decision made in _DevOps Dojo: Ros Node Config _ see DevOps Dojo: ROS Node Configuration - Update Parameter Contribution Guidelines

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The Reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

• There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

[kaspermeck-arm]

I noticed that pre-commit.ci made some changes

• ae75f5f

I think this decreases readability and shouldn't be used for the JSON schema either. Is this how we want it to be implemented?

[github-actions]

Documentation URL: https://autowarefoundation.github.io/autoware-documentation/pr-367/
Modified URLs:

• https://autowarefoundation.github.io/autoware-documentation/pr-367/contributing/coding-guidelines/ros-nodes/parameters/

[kminoda]

Thank you for your contribution!

What is the current status of this PR? Is it ready for review?
Given that there is some comment like (Original content, will need updating) in the PR, and also some required CI is failing, is this PR still not ready for review?

[xmfcx]

@kaspermeck-arm thanks for the PR!

• I've changed the PR title to fix the semantic-pull-request CI test.
• Could you also fix the https://results.pre-commit.ci/run/github/429694350/1684157254.Y4cUa-XoSEuJwgn5B6SUBA pre-commit.ci checks too?
• I will mark this as draft until they are fixed, you can set it as "Ready for review" once the Required checks pass.
  • You can ignore the DCO one for now.

[xmfcx]

I noticed that pre-commit.ci made some changes

* [ae75f5f](https://github.com/autowarefoundation/autoware-documentation/commit/ae75f5f1a3bcd922ad401a905155bc289b90e351)

I think this decreases readability and shouldn't be used for the JSON schema either. Is this how we want it to be implemented?

It refactors them based on length.
I think for small number of items, this is fine.
For large number of items, it will turn them into multi-line lists anyway. So we don't need to take an action for this.

[kaspermeck-arm]

@kminoda, thanks for your comments!
We're currently working on the next DevOps Dojo which will address the ROS Node Launch files, so the comment there acts as a placeholder. Once the dojo is complete, I'll update this section. Is this approach OK?

@xmfcx, thanks for your comments!
I rebased the commit and updated it to follow the comments from markdownlint.

[kaspermeck-arm]

@kminoda @xmfcx

It seems as though some links are broken in some other files. What's the process of resolving things?

Thanks for your patience while I learn how Autoware PRs work!

[xmfcx]

It seems as though some links are broken in some other files. What's the process of resolving things?

You can ignore them, they can be fixed later. Right now all Required CI tests are passed.

Thanks for your patience while I learn how Autoware PRs work!

Thanks for working on this, I will review as soon as possible.

[kaspermeck-arm]

@xmfcx @kminoda

Any chance this can get merged before our next Open AD Kit meeting on May 25th? Thanks!

[kaspermeck-arm]

@kminoda

DevOps Dojos is the way we structure work in the Open AD Kit WG. Each dojo addresses a certain topic, in this case the ROS parameters.

Find our Kanban board here.

[mitsudome-r]

Just as a general comment, I found the guideline a bit hard for the developer to understand what they should do when they write parameter related codes.
Maybe in the overview section at the top, we might want to mention the summary of items that developer should be aware of before going into the details, including:

• When they write ROS code (c++ or Python), they should be selecting the correct parameter declaration function. (to not have default parameter hard coded in the code)
• Have an example ROS yaml parameter under config directory in the package if the package contains an executable node.
• Have JSON schema file placed next to the example ROS yaml file, which is used to define the further details of each parameters.

[kminoda]

As an developer who have not followed the AWF DevOps discussions, here are some high level comments on this modification:

1. Could you elaborate more on the background of this documentation? As a newbie in this discussion, I could not get the whole idea until following the latest thread in this PR (especially the comment by mitsudome-san). Maybe adding some phrase like "to have a single source of truth" helps. IMHO this section should concisely answer why this JSON Schema is added in the first place.
2. Putting this section at the very top of this page is confusing. Consider moving this section to the last of this page or creating a separate page for this.

Comments are also welcome from @kaspermeck-arm and others as well!

[kaspermeck-arm]

@xmfcx @mitsudome-r

Thank you for your engagement on this topic!

I would like to propose the following:

• The PR is merged as is
  • The new guidelines improve the current guidelines
  • @mitsudome-r, once merged, please make changes you see appropriate to improve the guidelines
• @ambroise-arm and @kaspermeck-arm will start working on the GitHub Actions to validate the schemas and parameter files
  • DevOps Dojo: CI - JSON Schema Validation of YAML Parameter File(s) autoware-github-actions#232
  • Tips/ideas on how to do this from @xmfcx and @doganulus will be very helpful
• @HamburgDave reaches out to the community asking people to start adopting ROS packages to the new guidelines
  • Find list of ROS packages here under tab "ROS Node Config"
  • This is the reason why Open AD Kit needs the new coding guidelines to be live

Working in parallel will enable us to make progress faster! What do you think?

[kaspermeck-arm]

@isamu-takagi

Please at least reflect this comment. The order of chapters is confusing. I think the natural order for developers is:

* Declare parameters
* Create parameter file
* Launch node with parameter file
* Create parameter schema file (JSON Schema is the means, use the purpose for chapter name)

I suggest that we merge this PR as it is as it is a big change from the current coding guidelines. If you would like to change the order of the sections, create a PR with the proposed changes.

@xmfcx has agreed to this approach, see https://discord.com/channels/953808765935816715/953877118415151205/1113117316843044974.

Can we agree on this approach and merge this PR?

[isamu-takagi]

Please at least reflect #367 (comment). The order of chapters is confusing. I think the natural order for developers is:

@mitsudome-r @xmfcx At least two reviewers have pointed this out. Is there any reason you want to ignore this and merge? I can't understand why they would refuse to make a simple change, just swap the order of the chapters.

[kaspermeck-arm]

@isamu-takagi

Having the definition in place prior to creating the parameter file is also an option, there is nothing wrong with it, it's a preference. And I do want to highlight that the coding guidelines have already been approved.

I am not opposed to making changes to the coding guidelines once this PR has been merged. The purpose is to make it easier for the developers. Once merged, we can ask the community for feedback on the topic of usability and iterate on the coding guidelines until the community is satisfied with them.

[isamu-takagi]

@kaspermeck-arm
It is your job to reflect the reviewer's point out. Don't impose it on others.

[isamu-takagi]

@kaspermeck-arm
In fact, two reviewers pointed out that it was confusing, and it took no time at all to fix it. why not do that?

[mitsudome-r]

@isamu-takagi If you are concerned to have a page that might be potentially confusing for some people, maybe I can create a PR to Kasper's documentation before merging his.

[isamu-takagi]

@mitsudome-r I'm just saying follow the normal review process. According to Autoware's guidelines, it's the pull request owner's job to reflect the reviewer's comments. But if you specifically allow merging this PR, that's fine. I will update later. However, if this is accepted for everyone, the pull request guidelines will not work.

[mitsudome-r]

@isamu-takagi, thanks for your concern.

We have discussed this in Open AD Kit call today with @kaspermeck-arm.
From the discussion, we have agreed that @kaspermeck-arm to make the following changes to address the feedback:

• change the order of sections as mentioned in this comment
• Add some details about rationale behind adding JSON schema file.

@kaspermeck-arm, for the second point, I would leave it up to you how you make the changes in the end, but here are some of my suggestions:

• Perhaps using the images in this comment might be useful.
• optionally, listing reference links to past discussions might be also helpful for readers to track further details.

[kaspermeck-arm]

@isamu-takagi @kminoda @mitsudome-r

Thanks for the discussion and correspondence! I've made updates to the coding guidelines as was decided in the Open AD Kit WG meeting today (June 21st).

[isamu-takagi]

@kaspermeck-arm Thank you for updating. The latest documentation makes it easier to understand the benefits of schema files. I wait for @mitsudome-r -san's approval.

[taikitanaka3]

@kaspermeck-arm
Thank you for PR, but I think it's quite hard to see this PR for other maintainers too because

• There are a lot of diffs for one small yaml file like this PR.
• As @isamu-takagi -san says, most of Maintainer doesn't know the correct format of this json schema.
• We don't know the way to validate values you added to this PR. May there should be some ways to test with CI.

[kaspermeck-arm]

@kaspermeck-arm Thank you for PR, but I think it's quite hard to see this PR for other maintainers too because

* There are a lot of diffs for one small yaml file [like this PR](https://github.com/autowarefoundation/autoware.universe/pull/3992).

* As @isamu-takagi -san [says](https://github.com/autowarefoundation/autoware-documentation/pull/367#issuecomment-1569417302), most of Maintainer doesn't know the correct format of this json schema.

* We don't know the way to validate values you added to this PR. May there should be some ways to test with CI.

@taikitanaka3 - Since a few weeks ago, GitHub Actions have been put in place to validate the schema and the parameter file. Regarding the code review, what's expected from the owner is:

• ensure that the parameter description is accurate
• ensure that the bounds are correct

The rest should be handled by the CI and owner of the PR.

How does this sound?

[taikitanaka3]

@kaspermeck-arm
Thank you for your comment I understand CI is included in github action but still two concerns left.

• There are a lot of diffs for one small yaml file refactor(joy_controller): rework parameters autoware.universe#3992.
• As @isamu-takagi -san docs(parameters): add DevOps Dojo: ROS Node Config documentation #367 (comment), most of Maintainer doesn't know the correct format of this json schema.

[doganulus]

@taikitanaka3 We proposed schemas in the OpenAD Kit with @kaspermeck-arm and others.

Schema is the description of the parameter file. You explain the parameter file to a computer. What is the type, what are allowed values, what is its structure? Of course, it will be longer than the instance. I think your first concern is a misunderstanding of the purpose of the schema files.

The second is more about safety culture. Previously parameter files have been unchecked, uncontrolled, and untested. This is a safety problem. Think the schema is a safety belt in your vehicle. No one likes a safety belt until there happens a crash, and it saves your life. The infamous example of the Ariane5 rocket crashed in 1996 because of a single type error in their software. Maintainers are responsible for not letting this happen for Autoware. If we tell them in that way, they would immediately start to learn the correct format and every other safety feature we will introduce.

[kaspermeck-arm]

@kaspermeck-arm Thank you for your comment I understand CI is included in github action but still two concerns left.

* There are a lot of diffs for one small yaml file [refactor(joy_controller): rework parameters autoware.universe#3992](https://github.com/autowarefoundation/autoware.universe/pull/3992).

* As @isamu-takagi -san [docs(parameters): add DevOps Dojo: ROS Node Config documentation #367 (comment)](https://github.com/autowarefoundation/autoware-documentation/pull/367#issuecomment-1569417302), most of Maintainer doesn't know the correct format of this json schema.

@taikitanaka3

I thought I addressed your two concerns in my comment above.

To me this sounds like you and @isamu-takagi would benefit from understanding schemas better. I would recommend exploring https://json-schema.org/learn/ which has very useful information about JSON Schema. In short, a schema is meta-data, that is, data about data, creating the structure for the parameter file. Let me know if there's anything the Open AD Kit WG can do for you to help assist your learning!