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

Automated generation of sample config files #661

Closed
wants to merge 6 commits into from

Conversation

agarciamontoro
Copy link
Member

Summary

Just some Friday night fun to generate the sample config files automatically, since I found some discrepancies earlier this week and thought this was going to get out of sync rather sooner than later even if I fixed it.

So I created a command to generate the sample files automatically using the default values in the structs (making sure that slices had at least one element, so we could showcase all possible values), and added it to make assets so that CI will fail if it detects some change there.

Next step (next Friday, maybe?): use ltassist check to check the docs in CI as well.

Ticket Link

--

@agarciamontoro agarciamontoro added the 2: Dev Review Requires review by a core committer label Nov 3, 2023
@agarciamontoro
Copy link
Member Author

That last commit should fail because I didn't run make assets. Let's see 👀

@agarciamontoro
Copy link
Member Author

It failed, yay! Let's see now after make assets.

@agarciamontoro
Copy link
Member Author

agarciamontoro commented Nov 3, 2023

CI didn't run, huh? Ah, it's running now, it just took a while 🤷

@agarciamontoro
Copy link
Member Author

And it worked!

I acknowledge that we lost some specific examples (as in simplecontroller), but I'd argue that specific examples should live elsewhere, and that the sample config files should simply showcase all the possible fields and default values. Let me know if that doesn't make sense for you guys.

@isacikgoz
Copy link
Member

So basically we are creating empty configs with default values if possible, rather than with examples, not sure if this was we semantically desired. cc @streamer45?

@agarciamontoro
Copy link
Member Author

So basically we are creating empty configs with default values if possible, rather than with examples

Yes, but I'd argue that the most important thing here is that the configs are now complete and will remain in sync from now on. But yes, we are losing some of the examples we wrote there. To be honest, the only one I'm actually concerned about is the list of coordinator queries; we rarely, if ever, use the rest. And I still believe that those examples, even if they're super useful, should live somewhere else, probably in a directory of template configurations tailored for different specific purposes, which is something we have discussed in the past. I can try to include that in this same PR so we don't lose any information when merging this one.

@agnivade
Copy link
Member

agnivade commented Nov 6, 2023

I think having pre-loaded default config files are very important for someone to get up and running quickly with the tool. Losing the coordinator queries is not ideal. I think you are thinking in the right direction: having a directory to store some template configs and then merge with the autogeneration code is ideal in my view.

@agarciamontoro
Copy link
Member Author

Neat, I'll work on that directory of templates then. Thanks for the discussion, guys :)

@agnivade agnivade removed their request for review December 7, 2023 04:31
@agnivade
Copy link
Member

agnivade commented Dec 7, 2023

Removing myself. Feel free to request a review when ready.

@agarciamontoro agarciamontoro removed the request for review from isacikgoz December 11, 2023 10:36
@agarciamontoro
Copy link
Member Author

Fair enough! I will add you guys as reviewers again when it's done.

@agarciamontoro
Copy link
Member Author

Closing this ancient PR. Will reopen if I have some spare time to spend on it

@agnivade agnivade deleted the sample.json.generation branch September 23, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants