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

Add sanitization to BranchNamer::DependencyGroupStrategy #7452

Conversation

TomNaessens
Copy link

@TomNaessens TomNaessens commented Jun 19, 2023

Summary

This MR adds sanitisation support to the BranchNamer::DependencyGroupStrategy. It addresses this fixme:

# FIXME: Incorporate max_length truncation once we allow user config

History

Reasoning

  • Although the max_length feature isn't exposed yet on Github, other users downstream are already using the max_length parameter (e.g. through dependabot-gitlab which is built on dependabot-common)
  • As we're still creating branches, the normal sanitization should happen, no matter which strategy is used:
    • Branches can always be too long
    • Branches should always follow git ref limitations
    • Characters in branch names should always be able to be restricted (e.g. through the seperator command)
  • In the DependencyGroupStrategy, special care is taken that dependency order and time yields a consistent name. As hashing a consistent string also yields a consistent string, it is safe to reuse the sanitization strategy from the SoloStrategy.

To address those, I created a common superclass (BranchNamer::Base) with some basic sanitization methods (at the moment, moved from the SoloStrategy).

@TomNaessens TomNaessens requested a review from a team as a code owner June 19, 2023 18:24
@TomNaessens TomNaessens force-pushed the add-sanitization-to-dep-group-strat-branch-namer branch from b81d419 to ed307b5 Compare June 19, 2023 18:30
Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

Thanks for this, this looks good to me!

@brrygrdn brrygrdn merged commit 19f4402 into dependabot:main Jun 21, 2023
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