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.

Sign up for GitHub

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

Jump to bottom

New commands: New-DbaDbFileGroup, Set-DbaDbFileGroup, and Remove-DbaDbFileGroup #7306

Merged
merged 9 commits into from
May 23, 2021
Merged

New commands: New-DbaDbFileGroup, Set-DbaDbFileGroup, and Remove-DbaDbFileGroup #7306

merged 9 commits into from
May 23, 2021

Conversation

lancasteradam
Copy link
Contributor

Type of Change

  • Bug fix (non-breaking change, fixes # )
  • New feature (non-breaking change, adds functionality, fixes # )
  • Breaking change (effects multiple commands or functionality, fixes # )
  • Ran manual Pester test and has passed (`.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/sqlcollaborative/appveyor-lab ?
  • Nunit test is included
  • Documentation
  • Build system

Purpose

  • New commands for creating, updating, and removing filegroups.
  • Made a minor fix for Disable-DbaFilestream to change RunningValue to RunValue

Approach

Basic commands for filegroup create, update, remove. All feedback is welcome.

Commands to test

See the command examples and the integration tests.

@potatoqualitee
Copy link
Member

Thank you, Adam! Will hopefully take a look this weekend.

jpomfret
jpomfret reviewed May 15, 2021
View reviewed changes
Copy link
Collaborator

@jpomfret jpomfret left a comment

Choose a reason for hiding this comment

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

This is great work - few ideas\comments for discussion. Thanks for your contributions!

functions/Set-DbaDbFileGroup.ps1
.PARAMETER Default
Specifies if the filegroup should be the default. Only one filegroup in a database can be specified as the default.

.PARAMETER ReadOnly
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there or should there be a way to set to turn ReadOnly back off?

functions/Set-DbaDbFileGroup.ps1 Outdated
Specifies the filegroup should be readonly.

.PARAMETER AutoGrowAllFiles
Specifies the filegroup should auto grow all files if one file has met the threshold to autogrow.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question - how do you turn this off? Do we need that?

functions/Set-DbaDbFileGroup.ps1 Outdated
Specifies the filegroup should auto grow all files if one file has met the threshold to autogrow.

.PARAMETER InputObject
Allows piping from Get-DbaDatabase.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if this should be piping from Get-DbaDbFileGroup instead of Get-DbaDatabase... thoughts? discussion?

functions/Remove-DbaDbFileGroup.ps1 Outdated
[DbaInstanceParameter[]]$SqlInstance,
[PSCredential]$SqlCredential,
[string[]]$Database,
[string]$FileGroupName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if FileGroup could accept multiple filegroups to be deleted at once?

functions/New-DbaDbFileGroup.ps1 Outdated
.PARAMETER Database
The target database(s).

.PARAMETER FileGroupName
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you rename this to just FileGroup for all three functions please? We tend to drop the name.

@lancasteradam
Copy link
Contributor Author

Thanks @jpomfret, much appreciated. I'll look into those items.

@lancasteradam
Copy link
Contributor Author

@jpomfret, thanks again for the feedback. I've incorporated all of the suggestions:

  1. Renamed the param for Filegroup.
  2. Added support for multiple filegroups names to be passed in for the Set and Remove commands.
  3. Added pipeline support for filegroup objects.
  4. Added support to set a filegroup as read/write.
  5. Added the option to turn off the auto grow of all files.

jpomfret
jpomfret reviewed May 20, 2021
View reviewed changes
functions/Set-DbaDbFileGroup.ps1 Outdated
Comment on lines 103 to 104
[switch]$ReadOnly,
[switch]$ReadWrite,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be better to handle these as a single param instead of two switches... this currently doesn't fail and seems to set it to ReadWrite.
Set-DbaDbFileGroup -SqlInstance mssql1 -Database DatabaseAdmin -FileGroup test2New -ReadWrite -ReadOnly

You could leave it as a switch $ReadOnly and people can use $ReadOnly:$False to it back to ReadWrite.. or perhaps make it a string that accepts Enabled or Disabled as values?

Tagging @potatoqualitee and @wsmelton - do you have preferences or do we have a standard we should follow here?

functions/Set-DbaDbFileGroup.ps1 Outdated
Comment on lines 103 to 106
[switch]$ReadOnly,
[switch]$ReadWrite,
[switch]$AutoGrowAllFiles,
[switch]$AutoGrowSingleFile,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for these two, perhaps $AutoGrowAllFiles with Enabled\Disabled options?

@wsmelton
Copy link
Member

This needs to be closed and created as PR for each command.

@wsmelton
Copy link
Member

It is easier to review single command than trying to review multiple into one. Believe this has been mentioned before but may be wrong.

@jpomfret
Copy link
Collaborator

@wsmelton - happy for that to happen going forward, but we already have a lot of comments\discussion on this PR. Having them in one seemed to make sense for me to pull it down to my machine to test as well.

Do you have a preference on how ReadOnly and AutoGrowAllFiles should be handled?

@jpomfret jpomfret closed this May 21, 2021
@jpomfret jpomfret reopened this May 21, 2021
@jpomfret
Copy link
Collaborator

Apologies on the close\reopen - miss press on the app

@potatoqualitee
Copy link
Member

These three related commands seem reasonable to me as one PR. I'd prefer making it more convenient for contributors when possible, and allow multi files PRs unless it doesn't make sense for an unforeseen reason 🙇

@jpomfret
Copy link
Collaborator

Makes sense to me @potatoqualitee - thanks.

@lancasteradam - as for the param decisions... I think I'm leaning towards the best option being to make them both switches that when used set the property, and to unset it $ReadOnly:$false ... what do you think? Feels like that fits with SSMS having checkboxes?

image

@lancasteradam
Copy link
Contributor Author

@jpomfret, I agree, that sounds good. I'll make those changes.

jpomfret
jpomfret approved these changes May 23, 2021
View reviewed changes
Copy link
Collaborator

@jpomfret jpomfret left a comment

Choose a reason for hiding this comment

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

looks good - thanks!

@jpomfret
Copy link
Collaborator

unrelated test failures

@jpomfret jpomfret merged commit 33fb9f2 into dataplat:development May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpomfret jpomfret jpomfret approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lancasteradam @potatoqualitee @wsmelton @jpomfret