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

feat: remove the --devmode runtime option #1127

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jcrossley3
Copy link
Contributor

Fixes #1126

From what I can gather from the code, --devmode essentially did 2 things: 1) ran the db migrations, if any, and b) fired up an embedded OIDC server.

The concept of --devmode is a holdover from version 1, but since version 2 introduced the concept of "PM mode", having both modes is a source of confusion, and we have too many runtime parameters, anyway.

I also removed some public fn's that didn't seem to be used. It's difficult enough to understand our OIDC config code without it being conflated with a "dev mode". I assumed from the code that our dev mode defaults were reasonable defaults whether dev mode was a thing or not.

From what I can gather from the code, --devmode essentially did 2
things: 1) ran the db migrations, if any, and b) fired up an embedded
OIDC server.

The concept of --devmode is a holdover from version 1, but since
version 2 introduced the concept of "PM mode". Having both modes is a
source of confusion, and we have too many runtime parameters, anyway.

I also removed some public fn's that didn't seem to be used. It's
difficult enough to understand our OIDC config code without it being
conflated with a "dev mode". I assumed from the code that our dev mode
defaults were reasonable defaults whether dev mode was a thing or not.

Signed-off-by: Jim Crossley <[email protected]>
@jcrossley3 jcrossley3 requested a review from ctron January 7, 2025 19:28
@chirino
Copy link
Contributor

chirino commented Jan 7, 2025

I was using --devmode to enable auth for testing with an external DB config. What options would I migrate to?

@jcrossley3
Copy link
Contributor Author

I was using --devmode to enable auth for testing with an external DB config. What options would I migrate to?

I would expect you'd use the same CLI options, but with --embedded-oidc instead of --devmode.

@carlosthe19916
Copy link
Member

I was using --devmode to enable auth for testing with an external DB config. What options would I migrate to?

I would expect you'd use the same CLI options, but with --embedded-oidc instead of --devmode.

With the risk of being wrong I would say it will also need cargo run --bin trustd db migrate otherwise the DB tables won't be created. I also wonder if cargo run --bin trustd db migrate is needed only the first time I start the DB or that is a command that needs to be executed every time there are changes in the DB layer of Trustify

@jcrossley3
Copy link
Contributor Author

True. I was removing the "auto migration" feature of api --devmode in the same spirit of how #1063 removed the "auto import" feature. It all comes down to whether we want behavior in subcommands or options. Personally, I can learn to live with either or, but I don't love when we have both.

I'm willing to make an exception if it's useful, though. I would just prefer that the option name is specific, e.g. --migrate rather than generic, e.g. --devmode. That way, we don't have to keep trying to remember "wtf dev mode does".

@jcrossley3
Copy link
Contributor Author

Also, we might consider introducing a dev_mode fn similar to our pm_mode fn. That way, its behavior is explicit and contained within a single fn, rather than spread out through the code in various condition statements.

I find PM mode useful as a developer, but it's possible other developers don't, and we need to come up with some other bag of default features to facilitate their dev cycle.

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.

Unable to start Trustify (with importers) while using my own DB + OIDC provider
3 participants