dataplat / dbatools

🚀 SQL Server automation and instance migrations have never been safer, faster or freer
https://dbatools.io
MIT License
2.45k stars 797 forks source link

Candidates to remove #4102

Closed potatoqualitee closed 5 years ago

potatoqualitee commented 6 years ago

There are some commands that perhaps shouldn't belong. Are there one or more commands that come to mind that we should consider removing for 1.0?

This question is open to all and would also like @niphlod's input.

potatoqualitee commented 6 years ago

Long ago, someone mentioned Test-DbaOptimizeForAdHoc but then I saw a ton of people use/like it in our referrals.

niphlod commented 6 years ago

starting from the fact that input is sought to reduce the maintenance burden (because with 50 active devs this wouldn't be an issue whatsoever)...

first round could be anything GUI-related or interactive (e.g. show-dbadatabaselist). second round anything still not healthy and not touched in 300 days (enableexception and aliases do not count). Roughly it means that nobody wants to "adopt" it. third round (with a leeway, but let's fix one or we won't get to 1.0 ever) anything not tested (and yes, I know something cannot be tested but it doesn't suffice for every function).

This is of course if people don't use it, which is kinda unknown since we don't have statistics or "likes" :D

potatoqualitee commented 6 years ago

first round could be anything GUI-related or interactive (e.g. show-dbadatabaselist).

Show-DbaDbList isn't too hot of a command, but if it's written once, nbd? I can't limit the toolset to only what I'm committing to doing, and I'm always ready for (but dreading) a major contributor leaving.

second round anything still not healthy and not touched in 300 days (enableexception and aliases do not count). Roughly it means that nobody wants to "adopt" it.

I'm having a hard time with this one. If it's not touched, that could mean it's perfect 😇 I'm very wary of dismissing any commands because they weren't adopted. I love most of my 450 children.

third round (with a leeway, but let's fix one or we won't get to 1.0 ever) anything not tested (and yes, I know something cannot be tested but it doesn't suffice for every function).

I'd rather drink gallons of yerba mate spiked with espresso and chew packs of nicotine gum to propel myself into a 5-day test-writing bender than give up any commands just because they aren't tested.

I'm okay with removing useless commands, I'd prefer not removing unloved commands.

niphlod commented 6 years ago

don't take this towards functions you are the author of. we mostly adore you for everything that got created within and around dbatools 😍 However, IF something is gotta go, these are my rationales.

first round rationale: As for the "tooling that module provides" I'd go towards being consistent. We do lot of batch processing, cms-wise gets and sets, massive migrations, copy-world and as such. Interactiveness and GUI are quite apart from the vast majority of the module.

second round rationale: read carefully (I'll be doing in CAPS but I'm not yelling at all).... if it's HEALTHY and not touched in 300 days, it's GOOD. if it's STILL NOT HEALTHY and not touched in 300 days, there's no "traction" to make it "GOOD". Remember that we (core devs) formalized what we wanted for 1.0 on Nov 2017 and made them known to "the public" on Jan 2018.

third round rationale: if it's not tested it can't be "collaboratively improved". tests are a PITA (to write, to run, whatever) and even I dropped out of the bandwagon (waiting for $ to spend on a proper test machine is just one of the current round of excuses). However, I, and that's a personal and strongly opinionated view, wouldn't use anything 1.0 not tested against my estate ever (except maybe for Get-*). Even in the "medium" run of 1 year, we can all see that no tests mean no agile, slowing down adoption, rushing bugfix releases and general discouragement in helping out by users.

potatoqualitee commented 6 years ago

LOL thank you 🤗 Each command that is merged, with the exception of Backup/Restore that I could never handle myself, I adopt in the event that it's abandoned. I'm always eager for someone to take ownership, however. So it' not that I wrote it, but that we accepted it.

  1. You make a good point, and perhaps the Show commands can be moved to a dbatools-gui module. It wouldn't work in Linux anyway, so these are indeed good candidates.

  2. I totally looked over healthy! 😌 I'd be happy to consider those, but I think they're hard to find because a few have been fixed up but not labeled as fixed. I was considering going thru those next weekend.

  3. If I can hook you up with a rig, can you commit to a certain number of tests? Cuz, for you, I'll be happy to explore doing that if euro prices are ~ to USA prices 💞 🇮🇹

niphlod commented 6 years ago
  1. thanks for the proposal but winter is coming (and with it my birthday, and with that the rig). To be fair my brain got drained by other things at work and I could not spend too much time as a DBA as I would have wanted. With the rig done, it means that I'll be too damn sad to have spent such $ to be left alone and it'll push me having fun again with dbatools ( #illbeback )
wsmelton commented 6 years ago

There are options being looked at for Show-Command or equivalent to be brought into PowerShell Core.

If you are going to take the route to make dbatools fully cross-platform and write in stone that if a command won't work in PS Core then it won't go into the module; a lot of commands will have to be removed. Microsoft is not likely to port every SMO library over to PS Core. Any library you pulled from SSMS will be the primary ones you will not see in PS Core, because SSMS is not going cross-platform.

Simply because a command utilizes some pop-up or GUI does not mean it can't be in the module. Our primary user base is Windows environments. The purpose of the module is for both interactive management of SQL Server and automation. There is nothing wrong with mixing it.

wsmelton commented 6 years ago

read carefully (I'll be doing in CAPS but I'm not yelling at all).... if it's HEALTHY and not touched in 300 days, it's GOOD. if it's STILL NOT HEALTHY and not touched in 300 days, there's no "traction" to make it "GOOD". Remember that we (core devs) formalized what we wanted for 1.0 on Nov 2017 and made them known to "the public" on Jan 2018.

With PRs we are not enforcing our standards. If we don't want the extra leg work required to "fix" commands then they should not have been merged unless all of it was fixed initially. Then you reinforce with the author better practices for coding and they learn something in the process. If it was merged without applying the standards then are we going to punish the author for this and have the command removed?

niphlod commented 6 years ago

we don't wanna punish anyone but we need to face the fact that if 1.0 requires some standards to be followed it's either:

joshcorr commented 6 years ago

@niphlod if you don't mind I am willing to help with working on tests. It seems like that is one of the key things keeping the project from v1.0. I was about to start making a unit test for each cmdlet that doesn't currently have one (using the Dbatools template), so at least we are validating all currently known parameters out of the gate. I have a decent test environment as I have worked on some of the failover cluster cmdlet tests, so if we have some crazy integration testing that need done I can assist.

I don't know if I have much to say on which cmdlets should stay or go, but here is just my 2 cents. After creating simple unit tests for each existing cmdlet the policy moving forward could be that each new cmdlet requires a unit and acceptance test before the PR is accepted. Also maybe have a community survey for which cmdlets people are using (example: Check the box for each cmdlet you have used in the last 30 days, check all that apply). I know this is @potatoqualitee baby, but having a feedback loop from the community like this (without having any use metrics) would help adjust priorities on which cmdlets should be focused on. Again just my 2 cents. 🥂

wsmelton commented 5 years ago

Get-DbaRunningJob should be gone...all this does is create a custom object of a subset of properties from the $server.JobServer.jobs class. Get-DbaAgentJob returns the full SMO object and has the same information, there is nothing special being done in that running job command outside of just filtering.

potatoqualitee commented 5 years ago

thank you all for your feeedback 🙇 it's a good point about metrics. since we dont have anything reliable, I'll be closing this issue