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

DM-45394: Config from a single YAML file (except secrets) #380

Merged
merged 4 commits into from
Nov 12, 2024

Conversation

fajpunk
Copy link
Member

@fajpunk fajpunk commented Nov 8, 2024

Phalanx changes are in this PR

@fajpunk fajpunk force-pushed the tickets/DM-44145/app-metrics branch 2 times, most recently from f7d59ff to e900069 Compare November 8, 2024 17:08
@fajpunk fajpunk changed the title DM-44145: Config from a single YAML file (except secrets) DM-45394: Config from a single YAML file (except secrets) Nov 8, 2024
@fajpunk fajpunk force-pushed the tickets/DM-44145/app-metrics branch 4 times, most recently from 6abdc0a to fe1dcb5 Compare November 8, 2024 19:32
@fajpunk fajpunk requested a review from rra November 8, 2024 20:00
@fajpunk fajpunk marked this pull request as ready for review November 8, 2024 20:00
@fajpunk fajpunk force-pushed the tickets/DM-44145/app-metrics branch from fe1dcb5 to 58f724e Compare November 8, 2024 20:01
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should also change the flock configuration and therefore the business configuration and all of those models to also support camel-case so the entire configuration can be given in camel-case for consistency. As long as you have populate_by_name set, both snake-case and camel-case will be supported. There's some trick to getting the API documentation correct (snake-case) that I used with Nublado... oh, right, you have to make separate model classes for the loaded configuration that enable the alias generator and inherit from the API models. Unfortunately, I don't know of a way to do it other than code duplication.

I'll leave it up to you whether to do that additional work. I probably would because the inconsistency of camel-case in some parts of the configuration and snake-case elsewhere really bugs me, but that might just be a me problem. :)

docs/development/idfdev.rst Outdated Show resolved Hide resolved
docs/development/idfdev.rst Show resolved Hide resolved
src/mobu/handlers/github_refresh_app.py Outdated Show resolved Hide resolved
@fajpunk fajpunk force-pushed the tickets/DM-44145/app-metrics branch 2 times, most recently from a63b1b4 to de43e19 Compare November 11, 2024 19:56
@fajpunk
Copy link
Member Author

fajpunk commented Nov 11, 2024

Ideally we should also change the flock configuration and therefore the business configuration and all of those models to also support camel-case so the entire configuration can be given in camel-case for consistency.

oh, right, you have to make separate model classes for the loaded configuration that enable the alias generator and inherit from the API models.

This ends up touching a lot of nested models. This commit allows camelCase everywhere that's necessary to specify flocks. I'm not sure how to specify non-aliased versions of this without duplicating the field specifications on a lot of models, since alias generator model config doesn't propagate to submodels (which makes sense). It would be great if we could somehow specify in the models "use the non-aliased name for OpenAPI specs please," but to your point, I don't think there is.

I agree that a mix of styles in the app config is nasty, but I'm less put off by the inconsistent style in all of the helm values, as long as all of the app config is snake case. I can justify that in my brain as "this is directly generating a file that gets fed to the app, which is a python app, which likes snake_case, whereas everything outside of this config value is a helm value that controls templating and stuff, so it should be camelCase because that's what helm likes."

I think the options are:

  1. Have everything that goes in the app config yaml be snake case and snake case only. That would also make it so that API requests and docs are snake case. This leads to the least complex app code and the least confusing API docs, but inconsistent helm value style.
  2. Have all of the necessary config be compatible with either style, but don't duplicate models. This leads to slightly more complex app code, an inconsistently-styled (though correct) API and docs, and completely consistent helm values.
  3. Have all of the necessary config be compatible with either style, and duplicate all of the necessary models and override all of the necessary fields. This leads to consistent styles for both the API and helm values, but is there a way to do it without so much code duplication?

@fajpunk fajpunk force-pushed the tickets/DM-44145/app-metrics branch 2 times, most recently from 01cc4fc to 37cd37b Compare November 11, 2024 23:05
@fajpunk
Copy link
Member Author

fajpunk commented Nov 11, 2024

Have everything that goes in the app config yaml be snake case and snake case only. That would also make it so that API requests and docs are snake case. This leads to the least complex app code and the least confusing API docs, but inconsistent helm value style.

Meh, that sorta stinks too, it leaks into the helm templates more than I thought: lsst-sqre/phalanx@85938fb

@fajpunk
Copy link
Member Author

fajpunk commented Nov 12, 2024

I'm going to defer the work to support camelCase for the autostart config. I agree it would be nice, but I can't immediately do it without duplicating a lot of code, and I'd like to move forward with the metrics stuff. I'm going to merge this now under the auspices of "it doesn't make it worse."

@fajpunk fajpunk merged commit 3e9f712 into main Nov 12, 2024
4 checks passed
@fajpunk fajpunk deleted the tickets/DM-44145/app-metrics branch November 12, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants