-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - left a few minor comments, but nothing that would prohibit this from being merged.
Ran integration tests those were successfull as well.
.github/workflows/rust.yml
Outdated
- run: rustup component add rustfmt | ||
- run: cargo fmt --all -- --check | ||
|
||
clippy_check: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Install dbus dependencies | ||
run: sudo apt-get install libdbus-1-dev libsystemd-dev pkg-config libdbus-1-3 | ||
- name: Install systemd dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: maybe we should just call this "Install dependencies" - I think pkg-config is just neccessary for creating the APT package, so not strictly related to dbus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg-config is necessary for libsystemd-sys and openssl-sys. So it is a good idea to give the step a more general name.
.github/workflows/rust.yml
Outdated
@@ -39,8 +39,8 @@ jobs: | |||
runs-on: ubuntu-latest | |||
steps: | |||
- uses: actions/checkout@v2.3.4 | |||
- name: Install dbus dependencies | |||
run: sudo apt-get install libdbus-1-dev libsystemd-dev pkg-config libdbus-1-3 | |||
- name: Install systemd dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
[[package]] | ||
name = "async-compression" | ||
version = "0.3.7" | ||
version = "0.3.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few dependency updates in here that are unrelated as far as I can tell?
Totally fine for me, just wanted to briefly point it out to make sure this was on purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was on purpose. There were several dependabot updates which forced me to rebase this branch and fix conflicts so I decided to just update everything and test the changes extensively.
|
||
/// Type of an entry in a changes list | ||
#[derive(Debug, EnumString, EnumVariantNames, PartialEq)] | ||
#[strum(serialize_all = "kebab-case")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
today I learned :)
…"Install dependencies"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #148