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

Remove Team REX dependabot rota spreadsheet #674

Merged

Conversation

Jongmassey
Copy link
Contributor

@Jongmassey Jongmassey commented Jan 6, 2025

Fixes #664.

@Jongmassey Jongmassey force-pushed the Jongmassey/remove-teamrex-dependabot-rota-spreadsheet branch from 8dce89e to da6c541 Compare January 6, 2025 21:54
In preparation for non-spreadsheet-driven rotas
Move the bits of SpreadsheetRotaReporter that
aren't spreadsheet-specific (i.e. the base rota
reporting functions) into a base class to be
extended for non-spreadsheet rotas.
@Jongmassey Jongmassey force-pushed the Jongmassey/remove-teamrex-dependabot-rota-spreadsheet branch 2 times, most recently from e89dec1 to d74d84d Compare January 6, 2025 22:05
@Jongmassey Jongmassey marked this pull request as ready for review January 7, 2025 11:11
@mikerkelly
Copy link
Contributor

mikerkelly commented Jan 7, 2025

Thanks, Jon. I particularly like the changes so far to people.py, I think the namedtuple and Enum approach is a big improvement. I've made some suggestions and comments, mainly with an eye towards extensibility as that seems important in this repo, up to you which you take onboard. I've approved as this PR fixes the issue as-is.

It's not really for this PR, but it's no longer actually Dependabot we are using in at least some of our repos right, so perhaps we should re-name this workflow and the text it outputs? If you feel like quickly doing that in this or a separate PR or raising an issue for it feel free or I can do one of those.


from workspace.utils.people import get_slack_username
from workspace.utils.people import TEAM_REX, People, get_formatted_slack_username
from workspace.utils.rota import RotaReporter


class DependabotRotaReporter(RotaReporter):
Copy link
Contributor

Choose a reason for hiding this comment

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

DependabotRotaReporter now reports a rota based on cycling a code-defined set of people. I think this is a helpful approach for simple rotas that's likely to be wanted again for future additional or updated workflows. For example, maybe team RAP would want to use it for exactly the same purpose (dependency updates rota, if they have/want that). So it could make sense to pull out a class into utils/rota.py so it's more discoverable than being hidden away here. Then if people have that use case they can use or extend this rather than re-implement it. I don't think there's much here that's specific to our use case, a small amount of data in the candidates, purpose, and possibly offset, see other comments, which can be set when instantiated in report_rota. Hopefully that's not too much of a change but if you'd prefer not to do it in this PR that would be okay.

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 feel like that is perhaps premature optimisation. I had a mind that the team rex slack rota could move to this model (in a separate PR) and at that point I'd do the move. For now, it's a single case and thus I don't think warrants generalising (for now)

this_monday = today - timedelta(days=today.weekday())
next_monday = this_monday + timedelta(weeks=1)

candidates = [r for r in list(TEAM_REX) if r != People.KATIE]
Copy link
Contributor

Choose a reason for hiding this comment

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

DependabotRotaReporter hardcodes some data it has: the iterable of people it applies to (TEAM_REX - People.KATIE). This should probably be an instance-level or class-level attribute. This fits with the generalized class approach mentioned in a comment above.

Copy link
Contributor

@mikerkelly mikerkelly Jan 8, 2025

Choose a reason for hiding this comment

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

This was resolved without change or comment so I'm unsure if you intended to change something or not. If you don't intend to change anything in this PR that's fine, carry on. I slightly prefer leaving it unresolved in that case so it's more visible if someone looks at this PR again (eg to see what comments weren't actioned).

@Jongmassey Jongmassey force-pushed the Jongmassey/remove-teamrex-dependabot-rota-spreadsheet branch from d74d84d to 9038772 Compare January 8, 2025 11:26
To allow for selection of static rota for this team.

Refactor dict of github-username to slack-username
into an enum of staff for ease of referring to
staff members by real-world name rather than
github username.
Select a member of Team REX based on the week number.

(Random selection not possible as it is impossible
to consistently and statelessly get next week's
person)
Refactor to use Person type for clarity and readability.

Co-authored-by Mike Kelly <mike.kelly@thedatalab.org>
@Jongmassey Jongmassey force-pushed the Jongmassey/remove-teamrex-dependabot-rota-spreadsheet branch from 9038772 to 0e5be13 Compare January 8, 2025 13:30
@Jongmassey Jongmassey enabled auto-merge January 8, 2025 13:31
@Jongmassey Jongmassey merged commit eda9ae6 into main Jan 8, 2025
5 checks passed
@Jongmassey Jongmassey deleted the Jongmassey/remove-teamrex-dependabot-rota-spreadsheet branch January 8, 2025 13:31
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.

Remove the need for a spreadsheet for the Dependabot rota
2 participants