e-mission / nrel-openpath-deploy-configs

Configurations for current OpenPATH deployments, published for transparency
BSD 3-Clause "New" or "Revised" License
2 stars 9 forks source link

JSON appender script #40

Closed niccolopaganini closed 8 months ago

niccolopaganini commented 9 months ago
niccolopaganini commented 9 months ago

issue here: https://github.com/e-mission/op-admin-dashboard/issues/75

Added a template for the time being

niccolopaganini commented 9 months ago

@shankari please let me know if I understood what you said #70 here correctly.

shankari commented 9 months ago

@niccolopaganini please see my response. I don't want all the files to be changed. Also, as you can see, this code has introduced several extraneous changes. The only changes to the files should be the ones that we want. You probably want to write in UTF-8 format with appropriate indentation to avoid extraneous changes.

shankari commented 9 months ago

At this point, you may want to submit a separate PR by making the changes manually so we can get this done today, and work on cleaning up the script later.

niccolopaganini commented 9 months ago

checklist:

  1. encoding format ✅
  2. only target files ✅
  3. see if it has any "side effects" - I could not find anything after reverting the changed I made
niccolopaganini commented 9 months ago

troubleshooted for like the last 25 minutes and had to force-push the changes. revert all the config files that don't need any change. Changes only applied by script to the JSON files that need change. Hopefully, this meets all the criteria.

shankari commented 9 months ago

@niccolopaganini this is much better - the files are in UTF-8. However, as I said in https://github.com/e-mission/op-admin-dashboard/issues/70#issuecomment-1744049354

The list of programs that should not exclude the user_id is:

Instead, you have excluded the user_id from just those programs. You need to flip this around.

There are still extraneous formatting changes.

As I already said:

The only changes to the files should be the ones that we want.

This means that the only change should be the line for data_trajectories_columns_exclude. Everything else should be unchanged. Otherwise this is hard to review, and git blame gets too messed up. If you can't figure out the JSON dump arguments to enable this, feel free to use jq to edit the files directly instead. If there is a reason why this doesn't work, please document it

niccolopaganini commented 9 months ago

I was thinking about how to tackle this problem and I found this to be a possible path:

I went through the actions in the repo to peek at what's happening there (I wanted to see if there is anything relatable but I couldn't). Is this a path I could take that would be fruitful @shankari?