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

Migrate main menu config to image display policy objects #471

Closed
wants to merge 1 commit into from

Conversation

athornton
Copy link
Member

This turns num_dailies, num_weeklies, and num_releases into three of the fields on a more generic image policy object.

This object will initially be used for deciding which images to show on the main menu and which to show in the dropdown. It can also be used for choosing images to reap.

It defines both a number (that is, an image count, equivalent to the current behavior) and an age field, exactly one of which must be specified. Implementation of age-based filtering will be in a future PR. At the moment the functionality present is equivalent to what we have with the individual num_ fields.

It is quite possible that we want to remove PrepullerOptions (part of the v1 external API) and replace it with the policy in SpawnerMenuOptions.policy.main directly. This is part of something to be discussed Thursday, Feb. 6, 2025 in co-work. That would simplify this code a little (as we could remove the code that generates PrepullerOptions from the image policy), but have refactoring implications in the services layer (as well as our published (but with no external consumers) API).

@athornton athornton force-pushed the tickets/DM-48682-config branch 2 times, most recently from 60d5057 to c88a1a8 Compare February 4, 2025 21:53
@athornton athornton requested a review from rra February 4, 2025 23:52
@athornton athornton force-pushed the tickets/DM-48682-config branch from c88a1a8 to ec7e579 Compare February 4, 2025 23:55
@rra
Copy link
Member

rra commented Feb 5, 2025

This is probably a design topic for co-work, but to mention here in advance of the co-work discussion, I don't think I understand why we're changing the configuration for the parts of the menu outside of the dropdown. My understanding of the problem was that we wanted to hide images from the dropdown that were older than some threshold. I didn't think we wanted to change the menu display outside of the dropdown to be age-based. So, I guess I was expecting a single new configuration setting specifying the cutoff date for the dropdown, and I'm not sure where this more comprehensive change is headed.

@athornton
Copy link
Member Author

This is probably a design topic for co-work, but to mention here in advance of the co-work discussion, I don't think I understand why we're changing the configuration for the parts of the menu outside of the dropdown. My understanding of the problem was that we wanted to hide images from the dropdown that were older than some threshold. I didn't think we wanted to change the menu display outside of the dropdown to be age-based. So, I guess I was expecting a single new configuration setting specifying the cutoff date for the dropdown, and I'm not sure where this more comprehensive change is headed.

Fundamentally: we want the ability to filter a set of images via either age or number of images.

Given the current structure of the RSP tag format and how the menu works, we want the ability to do that per-image-category (that's uncontroversial for the main menu, obviously). The big problem with a single date for the dropdown is exemplified by "we want to show a month's worth of dailies and 18 months' worth of weeklies." And once we have the ability to specify an age-per-category, then it feels silly to have two completely separate mechanisms for age and number.

So then what we want is a generic filter for a set of tagged images. It should be a single class (or stack of classes, I guess) which lets us winnow down a larger set of images. That same class can be used for the image reaper too, of course, and that's fundamentally what this is, with the parts that know how to do semver stripped out for now. (We're going to want that too some day, when we start getting more serious about non-sciplat-lab RSP-launchable images, for which we should allow calver or semver but not the glorious mess of RSP tags.)

@athornton athornton force-pushed the tickets/DM-48682-config branch from ec7e579 to f433f9a Compare February 5, 2025 19:06
@athornton
Copy link
Member Author

Per co-work discussion, this isn't how we're going to approach this feature.

@athornton athornton closed this Feb 10, 2025
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