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

CEP 6: Add Channel Notifications to conda #19

Merged
merged 15 commits into from
Jun 20, 2022

Conversation

travishathaway
Copy link
Contributor

This pull request contains a draft CEP for the implementation of a new "Channel Notifications" feature to the conda package manager. Please see the CEP itself for more information about the proposal.

Please use this pull request as a public forum for discussing the feature proposal as well as suggesting improvements.

Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

You offered several subcommand ideas conda (alerts|motd|notices). For the sake of the .condarc config argument, we should probably avoid any aliasing and just pick one so we can keep consistent terminology throughout.

I vote for alerts since those are a well-defined concept from the web with understood severity tiers (e.g. bootstrap and tailwindUI).

cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
@wolfv
Copy link
Contributor

wolfv commented Apr 26, 2022

In generally I think it could be interesting for channels to be able to contain more metadata.

For example, if the robostack channel could easily advertise that it relies on the conda-forge channel to work, that could be cool.
A channel metadata could further have some fields for description etc.

cep-6.md Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
@seibert
Copy link

seibert commented Apr 27, 2022

I think another useful thing to specify in this CEP is the expectation that the client should fetch and show notifications before fetching or parsing the repodata.json file. The reason for this is that if the repository is corrupted in some way, it would be nice for admins to quickly put up a notifications.json file explaining why and where to get more information while it is being fixed. Could be similarly useful if a backward incompatible change to the repo format ever needs to be made. A clever web server configuration could serve a special notifications.json file to clients before a certain version in order to notify the user of the compatibility issue.

@LtDan33
Copy link
Member

LtDan33 commented Apr 28, 2022

  1. What is the thought on priority for channels? In theory if you are pulling from many channels you could have quite a few. Is there any limit to the number of alerts?
  2. It was also pointed out that the “expiry” naming could perhaps be a little clearer.
  3. Also I think I like the idea of having a dedicated notification json, do we want any qualifiers for specific client/conda metadata? For ex:
  • Having an “arch” field where the alert would only show if its a windows user in the case for a specific issue with users on that platform for example
  • Alert users of a conda version becoming EOL. We could add “conda_version” to target that.
  • A critical vulnerability for a user using a specific package.

There are always “what about…” so I’m all for keeping it simple and then expanding, but trying to think through other common and straightforward use cases that currently would add a lot of value. There are some implementation details on if there would be any parsing of the user agent to only return the “correct” alerts.json to the client, or if the server would throw back everything and let the client figure out what it wants to show.

Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

I like where this is going, and just have some wording advice:
We need to be careful about framing this system as emergency-use only and use a more neutral word to communicate its diverse use cases.

As such I would recommend to (from the list of terms used in this text: "notifications", "alerts", "notices", "messages", "news") to stick to one that is a good compromise between length (because of the use as a subcommand) and neutrality. IMO that's "notices".

cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
travishathaway and others added 4 commits April 28, 2022 13:18
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
Co-authored-by: Jannis Leidel <jannis@leidel.info>
@travishathaway
Copy link
Contributor Author

@LtDan33 you bring up a good point about the number of messages being shown. When writing this I assumed that people may have about 4 or 5 channels in active use (e.g. defaults, conda-forge, bioconda, internal-corporate-one, internal-corporate-two). With ten or more, these messages could get pretty unwieldy.

To remedy this, I suggest we rely on channel priority like you suggested. For example, we could only display the top five channel notifications and instruct our users to read the full output by running conda notices.

I think the arch and conda_version fields would be a great addition to the message JSON Schema definition. These will be clearly marked as optional though.

Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Minor suggestion to use the same header whether a display_notices notice or if its conda notices notices. This should also make it easier to identify the notice's channel.

cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
@kenodegard
Copy link
Contributor

I suggest we rely on channel priority like you suggested. For example, we could only display the top five channel notifications and instruct our users to read the full output by running conda notices.

Let's assume a user has 5 channels defined (A-E). Channel A defines 2 critical notices and one info notice, channel B/C both define one info notice, and D/E both define one critical notice. What notices do we prioritize if we show the top 5 notices?

  1. Display the most severe (and oldest) notice for the first 5 channels. Only one notice per channel.
  • A shows one critical notice
  • B shows one info notice
  • C shows one info notice
  • D shows one critical notice
  • E shows one critical notice
  1. Sort all notices by severity followed by channel priority. Display the top 5 notices.
  • A shows two critical notice
  • D shows one critical notice
  • E shows one critical notice
  • B shows one info notice

I don't think either of these is better than the other so it's probably best to only display one notice? That way there's no confusion as to what notice would be displayed. We could just include a statement that there are additional notices, and to run conda notices to see all of these notices.

Co-authored-by: Ken Odegard <kodegard@anaconda.com>
@travishathaway
Copy link
Contributor Author

@kenodegard You bring up an interesting point. I would err on the side of sorting by "priority", "channel order". We can play around with this in the initial implementation too.

In any case, we should definitely show a message instructing users if there are more unread messages. Something like:

5 of 12 new messages run `conda notices` to view all

@LtDan33
Copy link
Member

LtDan33 commented May 3, 2022

@kenodegard Good points. @travishathaway I like the thinking around making it explicit that there is value and a reason to run the command. Wordsmithing aside, I think we should make sure to call out any critical notices that aren't shown. Something like "X notices not shown, including y critical."

I'd also make sure we are consistent with the naming throughout around messages/notices/etc and just use one everywhere. Ubiquitous language ftw 😄

@travishathaway
Copy link
Contributor Author

travishathaway commented May 13, 2022

Hi all,

This is a recent video I have made demonstrating what this feature could look like once it is ready:

conda_channel_notifications.mp4

Looking forward to comments. If you have code specific comments, please take it over to the open PR I have current have for this: conda/conda#11462

cep-6.md Outdated Show resolved Hide resolved
cep-6.md Outdated Show resolved Hide resolved
@jezdez jezdez added the vote Voting following governance policy label Jun 8, 2022

up to date, audited 365 packages in 3s

34 packages are looking for funding
Copy link
Contributor

Choose a reason for hiding this comment

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

we need a funding nag, too!

cep-6.md Outdated Show resolved Hide resolved
@jezdez
Copy link
Member

jezdez commented Jun 13, 2022

Hi @conda-incubator/steering team, please make sure to review and vote on this CEP, thank you!

@mcg1969 mcg1969 self-requested a review June 13, 2022 14:19
@awwad awwad self-requested a review June 14, 2022 16:19
Copy link

@awwad awwad left a comment

Choose a reason for hiding this comment

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

(See comments above -- ISO 8601 expiry. Otherwise, looks good to me.)

@travishathaway
Copy link
Contributor Author

There were recent changes to this CEP. We have dropped the expiry field on individual message in exchange for an expired_at field. This new field will be set as an ISO 8601 timestamp string just like created_at.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Generally approve. Though saw one leftover bit below that might need updating

cep-6.md Outdated

## Resolution

TBD
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be replaced?

Copy link
Member

Choose a reason for hiding this comment

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

This might be replaced by the voting result, but it's optional, since the governance policy only explicitly mentions documenting the results in the pull request where the vote was conducted.

I would suggest to use the section if there would have been a controversial debate prior or during the vote. In this case I think it's fine to simply remove it when marking the CEP's status as "Accepted". Just my opinion though.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting maybe we should only include this section when it is needed then?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's not a bad idea, CEP 1 uses "Some section that may be included if appropriate for the proposal include." so I mostly see this as part of the cleanup process before merging the proposal, in case the CEP author(s) have added it.

@jezdez
Copy link
Member

jezdez commented Jun 16, 2022

Voting results

This was a standard, non-timed-out vote.

This vote has reached quorum (15 + 0 = 15 which is at least 60% of 21).

It has also passed since it recorded 15 "yes" votes and 0 "no" votes giving 15/15 which is greater than 60% of 15.

It should be noted that multiple requests for change were recorded in the pull request about minor implementation details that do not invalidate the previous votes. The author @travishathaway made the requested change.

@jezdez jezdez merged commit 7e4b87d into main Jun 20, 2022
@jezdez jezdez deleted the cep-6-channel-notification-feature branch June 20, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vote Voting following governance policy
Projects
None yet
Development

Successfully merging this pull request may close these issues.