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

Create apt package management resource. #434

Merged
merged 16 commits into from
May 16, 2024

Conversation

JamesWTruher
Copy link
Contributor

PR Summary

This creates an package manager for apt
it includes some tests

PR Context

type: DSC.PackageManagement/Apt
properties:
packageName: rolldice
_exist: true
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if this value was defined as a parameter so instead of having two similar config, you could just pass in false (and default to true). See https://github.com/PowerShell/DSC/blob/main/dsc/tests/dsc_parameters.tests.ps1 for examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to leave that as an exercise for the reader

- name: apt_wget
type: DSC.PackageManagement/Apt
properties:
packageName: wget
Copy link
Member

Choose a reason for hiding this comment

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

packageName should probably also be a parameter

@@ -17,7 +17,7 @@ Describe 'Tests for listing resources' {
}

It 'dsc resource list --tags "<tags>" and --description "<description> work' -TestCases @(
@{ tags = 'linux'; description = $null; expectedCount = 1; expectedType = 'Microsoft/OSInfo' }
@{ tags = 'linux'; description = $null; expectedCount = 2; expectedType = @('DSC.PackageManagement/Apt', 'Microsoft/OSInfo') }
Copy link
Member

Choose a reason for hiding this comment

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

Since this test now fails on Windows and macOS, you'll probably need to modify this test. Simplest might be something like:

expectedType = if ($IsLinux) { @(...) } else { @(...)}

else
echo $output
fi
elif [[ "$1" == "set" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check if the resource is invoked as root? If I recall correctly, install and remove fail as normal users.

Copy link
Member

Choose a reason for hiding this comment

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

@michaeltlombardi perhaps we should have a general issue opened to add in the resource manifest when specific operations require elevated and DSC can fail fast

_exist=true
fi
if [[ $_exist = true ]]; then
apt install -y "${packageName}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this handle installing a specific version, since we have the version property? Or should version be marked as readOnly?

Copy link
Member

Choose a reason for hiding this comment

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

I think this resource should support specifying the version, but limit it to apt syntax

Copy link
Member

Choose a reason for hiding this comment

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

We can add this later

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

We can iterate and improve separately

@SteveL-MSFT SteveL-MSFT added this pull request to the merge queue May 16, 2024
Merged via the queue into PowerShell:main with commit 06e79cd May 16, 2024
4 checks passed
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.

3 participants