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.

@ctron
Copy link
Contributor

ctron commented Jan 13, 2025

From my experience in the past, we added to much "auto magic" to "devmode", not being able to control these flags/features individually. PM goes into the same direction. I personally use "devmode" with out "pm mode". Meaning external database, (sometimes) external OIDC. But I still would prefer to have a good subset of "defaults" that "devmode" brings.

I believe that it would be good to have:

  • PM Mode: Kind of as a "deployment model", running that all-in-one setup, including embedded infrastructure services
  • Dev Mode: As an opinionated way to run a single service
  • The rest: all individual options

PM mode could reuse "devmode" internally. All options that devmode (and pm mode) set, should be achievable by using individual flags too.

@helio-frota
Copy link
Collaborator

PM Mode: Kind of as a "deployment model", running that all-in-one setup, including embedded infrastructure services

@jcrossley3
Copy link
Contributor Author

From my experience in the past, we added too much "auto magic" to "devmode", not being able to control these flags/features individually.

Agree, strongly.

PM goes into the same direction.

I disagree, for one very important reason: it's easy to tell from the code exactly what PM mode does. It's all self-contained in a single function called pm_mode.

Dev mode, by contrast, is comprised of various conditional statements littered throughout the code. So it's difficult to tell exactly which features it enables and it's too easy for the "auto magic" you mentioned to creep in over time.

I personally use "devmode" with out "pm mode". Meaning external database, (sometimes) external OIDC. But I still would prefer to have a good subset of "defaults" that "devmode" brings.

Ok, but this whole exercise was motivated by the question, "what does devmode do?", which if we expose it as a command line option, is a perfectly reasonable question for any user to ask.

I believe that it would be good to have:

  • PM Mode: Kind of as a "deployment model", running that all-in-one setup, including embedded infrastructure services
  • Dev Mode: As an opinionated way to run a single service

I don't understand this phrase. What opinion? Where is it defined?

  • The rest: all individual options

PM mode could reuse "devmode" internally. All options that devmode (and pm mode) set, should be achievable by using individual flags too.

I do not believe devmode's value justifies its potential for confusion.

@ctron
Copy link
Contributor

ctron commented Jan 13, 2025

For me, ideally we'd have a set of flags that we can control. Things like auth, embedded oidc, DB, … all those things should be exposed as flags/arguments.

Using --devmode would change the defaults of such flags. Which might also mean to provide required information. But nothing more. You could still override, by manually providing arguments.

"PM mode" (independently on how this is triggered) would "call" (internally, like we do it today) the services/processes, and provide all arguments with that. Maybe (so that it's easier) actually making use of --devmode (internally).

@jcrossley3
Copy link
Contributor Author

What you're describing sounds reasonable. I think we could implement that behavior entirely within the trustd module. I see that work as best contained in a separate PR, and it doesn't preclude this PR from being merged: it removes the server option, some unused functions and renames "devmode" to "default", thereby enabling us to implement the functionality you describe on a clean slate.

Or I can just close this PR and we can go on with things as they are.

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
5 participants