autowarefoundation / autoware-github-actions

Apache License 2.0
17 stars 24 forks source link

fix: unmatching yaml file checking #293

Closed badai-nguyen closed 6 months ago

badai-nguyen commented 6 months ago

Description

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

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

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

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

xmfcx commented 6 months ago

But we actually want to check multiple param files that start with the base_name, that's the point of having a base_param name.

Why do you need this change?

xmfcx commented 6 months ago

example usage: https://github.com/autowarefoundation/autoware.universe/tree/44c6169d1fca13cd1035d459dd127758d5296595/control/joy_controller/config

4 different param.yaml files are managed by a single json schema.

badai-nguyen commented 6 months ago

@xmfcx Thank you for your comment. I understood that It was intentional setting. However, i think there are also some cases that more explicity definition of yaml file is needed. For example, https://github.com/autowarefoundation/autoware.universe/tree/main/perception/lidar_centerpoint/config If we create centerpoing.schema.json, it will check all other yaml files. Do you have any suggestion for that?

xmfcx commented 6 months ago

@badai-nguyen

If we create centerpoint.schema.json, it will check all other yaml files.

Then, you can avoid this by naming the schema and param files accordingly.

I dont know what those schemas and params do but you could name them according to their purposes:

There is no need to change the way it currently works.

Additional benefits

This would also make it easier to understand different kinds of param.yaml files too.

Right now lidar_centerpoint/config folder looks like a mess and it is hard to understand what they all stand for.

badai-nguyen commented 6 months ago

@xmfcx I see. Then I will close this PR and revise the schemla.