Skip to content

RFC: Allow '/' in distribution name - #1089

Closed
adalessandro wants to merge 1 commit into
aptly-dev:masterfrom
adalessandro:wip/adalessandro/distribution-allow-slash
Closed

RFC: Allow '/' in distribution name#1089
adalessandro wants to merge 1 commit into
aptly-dev:masterfrom
adalessandro:wip/adalessandro/distribution-allow-slash

Conversation

@adalessandro

Copy link
Copy Markdown

This is an RFC PR to follow up on the ongoing opened issue #115
Possible duplicated or already a declined feature. But it's not really clear why this is considered a bug in aptly and what would be the consequences of allowing slash / characters in the distribution name.

This reverts commit 1daa076.

Fixes #115

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

…-' while guessing. aptly-dev#110"

This reverts commit 1daa076.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
@codecov

codecov Bot commented Jun 27, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1089 (45b18e6) into master (c9f5763) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1089      +/-   ##
==========================================
- Coverage   52.11%   52.09%   -0.03%     
==========================================
  Files          73       73              
  Lines       11272    11267       -5     
==========================================
- Hits         5874     5869       -5     
  Misses       4832     4832              
  Partials      566      566              
Impacted Files Coverage Δ
deb/publish.go 62.51% <ø> (-0.22%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@sjoerdsimons

Copy link
Copy Markdown
Contributor

For reference we've been running with this revert for Apertis for quite a while now and things seem entirely happy ;) But as @adalessandro mentioned we may well be missing the background reason of why this was disallowed in the first place?

@randombenj

randombenj commented May 22, 2023

Copy link
Copy Markdown
Contributor

image

@sjoerdsimons

Copy link
Copy Markdown
Contributor

fwiw the one thing we noticed recently that when publishing a distribution with a / "under" another distribution then removing the top-level one will remove everything underneath :) So that was likely based on the assumption that distribution names under a prefix don't have a / (I'm not sure if aptly also protects against nested prefixes)

As a more practical example of what i meant above, for Apertis, we publish both the current repositories and snapshots with a setup as follows:

  • current: /apertis/dists/v2024pre
  • snapshot: /apertis/dists/v2024pre/snapshots/<timecode>

When dropping current it will actually wipe out everything under /apertis/dists/v2024pre which ofcourse then includes all published snapshots. This was only uncovered recently due to some transition/testing as we'd normally never want to delete the current published repositories and leave the snapshots :)

So for this RFC that's one thing to fix for sure; Based on our experiences thusfar that's likely the only issue with this RFC

@neolynx neolynx self-assigned this Jan 14, 2024
@neolynx

neolynx commented Jan 14, 2024

Copy link
Copy Markdown
Member

are you creating local repos with a / in the distributions, or do you use mirrors with such ?

@neolynx neolynx mentioned this pull request Apr 11, 2024
6 tasks
@neolynx

neolynx commented Apr 11, 2024

Copy link
Copy Markdown
Member

superseded by #1269

@neolynx neolynx closed this Apr 11, 2024
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.

Support for '/' in distribution names

4 participants