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

Fix for Issue 176 #177

Merged
merged 3 commits into from
Mar 30, 2022
Merged

Fix for Issue 176 #177

merged 3 commits into from
Mar 30, 2022

Conversation

blitzmann
Copy link
Contributor

@blitzmann blitzmann commented Mar 11, 2022 •
edited
Loading

Closes #176
Closes #146
Please let me know if there's any thoughts here. It is my understanding that $ParentVHD and ($Database, $LatestImage) are mutually exclusive. However, the codebase requires $Database to be set, so when using $ParentVHD it wont actually work (since there are no databases set). This fix basically looks up the database name if supplied a ParentVHD

I also added a new parameter, $ImageID, to be able to create a clone based on an image ID.

Also, I'm not sure what utility $LatestImage has. The code looks like this:

 foreach ($db in $Database) {

            if (-not $ParentVhd) {

                if ($LatestImage) {
                    $result = Get-DcnImage -Database $db | Sort-Object -Descending CreatedOn | Select-Object -First 1
                }

                # Check the results
                if ($null -eq $result) {
                    Stop-PSFFunction -Message "No image could be found for database $db" -Target $pdcSqlInstance -Continue
                }
                else {
                    $ParentVhd = $result.ImageLocation
                }
            }

$LatestImage seems to be required, otherwise $result is always unset. Is there a purpose for this? I propose it gets removed.

sanderstad and others added 3 commits March 5, 2022 18:35
@sanderstad
Merge pull request dataplat#169 from dataplat/development
6da7cbc 
Fix remote removal clone and recovery model of database
@blitzmann
Fix for dataplat#176 - If supplied a Parent VHD, we look up the datab…
5c564cd 
…ase for it.

Additionally, add a new parameter to generate a clone for an image ID, in case that is already known
@blitzmann
Add parameter documentation
970c3eb
@sanderstad
Copy link
Collaborator

The thought about -LatestImage was to make it easier for the user to use the latest image and didn't have to look it up.
Removing it would probably not be a good idea

@blitzmann
Copy link
Contributor Author

blitzmann commented Mar 17, 2022 •
edited
Loading

@sanderstad That makes sense, but it's not implemented in that way. There isn't actually a way to say "I want this image instead" besides giving it a $ParentVhd, but by doing so it makes the $LatestImage redundant

if (-not $ParentVhd) {
                if ($LatestImage) {

So if you provide a ParentVhd, it uses that regardless of what LatestImage is set to. If you don't use it VHD, then LatestImage MUST be set for $result to be populated. If $result isn't populated, then the function stops. There's no way in current master to be able to declare Database and select an image that isn't the latest one

This PR fixes that tho, by 1) allowing the script to work with ParentVhd and without a supplied Database value (as the example in the script shows), and 2) allowing you to set the image ID that you want to clone. I would propose, as another PR once this is merged, making either $ImageId or $LatestImage required, if $Database is set. I don't know how that would play with specifying multiple databases to clone (I would assume LatestImage would be required if specifying multiple databases, as there isn't an easy way to tie those databases with specific images).

Please let me know if I'm off base or not understanding the purpose behind something, but as it stands now (in master) there is no way of getting a specific image and LatestImage is simply redundant. That may change int he future tho

@sanderstad sanderstad merged commit bc03aee into dataplat:development Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanderstad sanderstad sanderstad 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.

New-DcnClone example does not work New-DcnClone how to create with a specific image?
2 participants
@blitzmann @sanderstad