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

Deprecation notice of removed "dba" container occurs too often, collapse into one message? #5246

Closed
1 task done
garvinhicking opened this issue Aug 8, 2023 · 6 comments
Closed
1 task done
Milestone

Comments

@garvinhicking
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Output of ddev debug test

Expand `ddev debug test` diagnostic information
(shouldn't apply so I try to not waste the ressources for this at this point)

Expected Behavior

Using ddev commands lets me work without interruption or duplicate messages

Actual Behavior

Getting duplicate warning messages about a deprecated functionality on every command like ddev launch or even ddev ssh:

WARN[0000] PhpMyAdmin (`dba`) is no longer part of DDEV core, please edit your `omit_containers` configuration to remove it
WARN[0000] PhpMyAdmin (`dba`) is no longer part of DDEV core, please edit your `omit_containers` configuration to remove it
PhpMyAdmin (`dba`) is no longer part of DDEV core, please edit your `omit_containers` configuration to remove it
PhpMyAdmin (`dba`) is no longer part of DDEV core, please edit your `omit_containers` configuration to remove it
PhpMyAdmin (`dba`) is no longer part of DDEV core, please edit your `omit_containers` configuration to remove it

Steps To Reproduce

In a ddev 1.22.x environment with a config of omit_containers: [dba] run any ddev command and see the duplicate messages.

Anything else?

dba was removed via #3534.

I do see the need of informing users if the dba-container was maybe referenced, but if I previously explicitly omitted the container, I don't really need the information that it has been removed? (and not 5 times; one time per project would be sufficient, and best only once and never again)

Our reason for not just adding omit_containers: [] to the configuration is, that we also have co-workers on older ddev versions, and if we change the ddev config to remove this omit_containers statement, those people will actually get dba installed for them.

We would love a way to either "mark a notice as read" to include in the configuration (harder feature, maybe not really woth effort), or better, just remove the information for an omitted container...?

(Once again, many thanks for the work on ddev, it can't be stated often enough)

@stasadev
Copy link
Member

stasadev commented Aug 8, 2023

That's a good idea!

There was already a certain duplication of warnings, for example:

util.Warning("User-managed %s will not be managed/overwritten by ddev", gitIgnoreFilePath)

when the message is shown again, when you run a post-start hook.

Suggestion: add some global cache (where the message is calculated to sha1 and stored) to util.Warning():

 // Warning will present the user with warning text.
 func Warning(format string, a ...interface{}) {
+	if globalCache.Has(format, a...) {
+		return
+	}
+	globalCache.Add(format, a...)
 	format = ColorizeText(format, "yellow")
 	if a != nil {
 		output.UserErr.Warnf(format, a...)

And make this globalCache active for a single run of the DDEV command (similar to a single PHP request).


An alternative, if the above will not be considered something of value:

Remove dba everywhere in the project configs and have people with the old DDEV run this:

ddev config global --omit-containers=dba

@rfay
Copy link
Member

rfay commented Aug 8, 2023

We won't be trying to solve this, since it's temporary. DBA has been removed, once people have it out of their config, the messages are gone. The reason you're seeing it more than once is you have it configured in various places, and there's also a message on ddev start right now that tries to help you figure that out as well.

Are you having trouble figuring out how to remove it from your config @garvinhicking ?

Chasing after a temporary problem by adding code complexity just isn't worth it.

@garvinhicking
Copy link
Contributor Author

My thing is, we cannot remove it, because colleagues are still running on older ddev versions, and I think the message is not really useful in first place? Why is it shown in a case, where I omit the package, that is now already omitted by default - it only requires work for you, when in reality the outcome is the same?

(But if you decide against changing it, I can be happily silent and we can close this, I just thought it adds a layer of uneeded verbosity and wanted to help improve it ;))

@rfay
Copy link
Member

rfay commented Aug 8, 2023

I guess there could be an argument for just not showing the message at all. That doesn't add complexity. What do you think @gilbertsoft ?

@rfay rfay added this to the v1.23 milestone Aug 8, 2023
@rfay
Copy link
Member

rfay commented Aug 8, 2023

I'm fine with just removing the warning. If anybody wants to take on this super easy PR it's welcome.

@fe-hicking
Copy link

fe-hicking commented Aug 8, 2023

Oh yes, I'd love to try! :-) Will report back here.

(should've provided this from the beginning, my bad)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants