Skip to content

allow empty artifact list in notifier#30

Open
etrombly wants to merge 2 commits into
marekzan:mainfrom
etrombly:empty_artifact_ids
Open

allow empty artifact list in notifier#30
etrombly wants to merge 2 commits into
marekzan:mainfrom
etrombly:empty_artifact_ids

Conversation

@etrombly
Copy link
Copy Markdown

For a long list of artifacts it can be a pain to track them in the config. This allows you to leave artifact_ids empty, and it will add all tracked artifacts.

@marekzan
Copy link
Copy Markdown
Owner

Hey @etrombly :),

thank you for your contribution!

I like the idea of being able to just tell a notifier to respect every defined artifact without needing to specify every artifact_id. But intuitively the "[ ] -> all" approach does not feel right, since it might go against what most users might expect from an empty / none value.

I see two options which could solve the API design here:

  1. "artifact_ids": "all"
  2. "artifact_ids": ["*"]

The first approach might be more idiomatic because we could allow something like:

#[derive(Deserialize)]
#[serde(untagged)]
pub enum ArtifactSelection {
    All(AllMarker),
    Specific(Vec<String>),
}

#[derive(Deserialize)]
#[serde(rename_all = "lowercase")]
pub enum AllMarker { All }

The second one would be way easier to implement since we just need to check for the wildcard string in the place where you already implemented the copy logic.

Currently I'd be fine with both. If you like to adjust this PR then I will merge it ASAP and if not, then I am happy to make the changes myself :).

@etrombly
Copy link
Copy Markdown
Author

I'm implementing the first option, but wanted your opinion on something. I need to update NotifierResponse in veno-web. I'd like to just use the new ArtifactSelection enum there so I don't need to duplicate the definition, but I'd need to implement ToSchema for it. I'm assuming you don't want to add utopia to veno-core though. Which option would you prefer?

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.

2 participants