Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

Extract groups from settings #12

Closed
wants to merge 8 commits into from

Conversation

loewenheim
Copy link
Contributor

This is the pueue-lib part of Nukesor/pueue#240. It makes the setting settings.daemon.groups optional so that a deprecation warning can be displayed when it is present in an old configuration. The groups map in State now covers both status (Running/Paused) and number of parallel tasks (which used to be in settings). The struct that contains this information is called GroupInfo, a name I'm not really happy with.

src/state.rs Outdated
@@ -127,11 +160,36 @@ impl State {
Ok(())
}

/// Get an immutable reference to the specified group in this state, if it exists, otherwise
/// return a failure message.
pub fn get_group<'a, 'b>(&'a self, group: &'b str) -> Result<&'a GroupInfo, Message> {
Copy link
Owner

Choose a reason for hiding this comment

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

The state really shouldn't have any knowledge about messages or anything that's not strictly state related. I tried to keep it as stupid and isolated as possible.
The logic for error/failure messages should be done in the daemon's domain.

As long as groups is public and there are no complex state related operations related to it, we don't need any getters for this.

@@ -168,15 +168,14 @@ pub struct EditResponseMessage {

#[derive(PartialEq, Clone, Debug, Deserialize, Serialize)]
pub enum GroupMessage {
Add(String),
Add(String, usize),
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Add(String, usize),
/// Used to create a new group, whilst having the option to initializing the amount of parall tasks.
/// (name, parallel_tasks)
Add(String, usize),

Copy link
Owner

@Nukesor Nukesor Oct 5, 2021

Choose a reason for hiding this comment

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

I'm thinking about whether we should make this a Option<usize>.
I guess, the daemon should responsible for the default of 1 for new groups.
That way, we keep the client stupid and simple.

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 had this as an Option initially. The reason I changed it is that in the client, I defined the -p option is defined like this:

    Group {
        /// Add a group by name.
        #[clap(short, long, conflicts_with = "remove", value_name = "NAME")]
        add: Option<String>,

        /// Remove a group by name.
        /// This will move all tasks in this group to the default group!
        #[clap(short, long, value_name = "NAME")]
        remove: Option<String>,

        /// The number of tasks the newly added group should run in parallel.
        #[clap(
            short,
            long,
            requires = "add",
            conflicts_with = "remove",
            value_name = "TASKS",
            default_value = "1"
        )]
        parallel_tasks: usize,
    }

So it's strictly speaking not optional. It's a bit awkward then to always wrap this in Some for the message, but it's obviously possible. The other option, of course, would be to make parallel_task an Option as well.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm actually in favor of moving the default to the daemon.
As long as the count is optional in the message as well, the handling shouldn't be too akward, as you can move/clone the option directly into the option.

CHANGELOG.md Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
src/state.rs Outdated Show resolved Hide resolved
src/state.rs Outdated
@@ -48,7 +70,7 @@ pub struct State {
/// All tasks currently managed by the daemon.
pub tasks: BTreeMap<usize, Task>,
/// All groups
pub groups: BTreeMap<String, GroupStatus>,
pub groups: BTreeMap<String, GroupInfo>,
Copy link
Owner

Choose a reason for hiding this comment

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

This will get interesting. I assume, that this will result in breaking tests and backward incompatible state parsing, as the deserialization of this shouldn't work at all.

It might be, that the project finally reached the point in time, where we have to introduce proper backward compatible deserialization.

Copy link
Owner

Choose a reason for hiding this comment

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

The test for restoring a state was previously extracted to pueue, as there is a lot of pueued specific domain logic related to that process.

I just added a super reduced test, which only tests that the old state can still be deserialized. Could you go ahead and quickly rebase on the newest master :)?

I assume that this test will then fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it fails. Do we have to write a custom deserializer for Group to make this work or is there an easier way? I'm not that knowledgeable about serde.

Copy link
Owner

Choose a reason for hiding this comment

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

That will need some research.
It'll probably result in having to maintain several old versions of state parsers.
Nukesor/pueue#216

It's a largely unsolved and tricky problem:
serde-rs/serde#1137

I'll try to find some time and join you, however I'm pretty short on time lately.

@Nukesor
Copy link
Owner

Nukesor commented Oct 5, 2021

Thanks for starting the work on this issue :)

@loewenheim
Copy link
Contributor Author

Thank you for your feedback, especially with regards to the changelog and the deprecation message.

@Nukesor
Copy link
Owner

Nukesor commented Nov 6, 2021

This issue became a lot more complex than expected.

I'm going to close this PR in favor of #16 and Nukesor/pueue#259

It looks like we won't be able to achieve 100% backward compatibility, but we're at least in the range of 90%.
The group command won't work until the daemon is restarted and the amount of parallel tasks per group will be lost on update.

@Nukesor
Copy link
Owner

Nukesor commented Nov 6, 2021

Anyhow. Thanks for your effort!

I'm sorry for taking this over so blatantly, but there were a few critical design decisions that needed to be done and I felt more comfortable doing those by myself :)

I hope you can understand this decision!

@Nukesor Nukesor closed this Nov 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants