Skip to content

DO NOT MERGE: tests: add some questionable but valid platform names #117

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.

Sign up for GitHub

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

Open
lengau wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −0
Open

DO NOT MERGE: tests: add some questionable but valid platform names #117

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions tests/unit/test_platforms.py
View file
Open in desktop

@mr-cal mr-cal Apr 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some thoughts:

  1. Platform names are used for the container name. Craft-providers sanitizes platform names before creating a container, so this isn't a concern.
  2. Platform names are used in the resulting artifact. So it may be prudent to only allow characters allowed in file paths. We could add a few extra characters and replace them. I think we are already doing this with @ or : symbols in Charmcraft.
  3. For usability, we could set a maximum limit. The artifact filename should be less than 255 characters (a common max), so this could drive the maximum limit.
  4. We could set a reasonable ruleset. Unless there is a real need to support the platform names in this PR, I'd be fine for something along the lines of:

Can contain alphanumerics, :, @, - and _, must start and end with an alphanumeric, max length is 64 characters.

@lengau lengau Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want 🇿🇦 to be a valid platform name.

Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,34 @@
)
for arch in craft_platforms.DebianArchitecture
),
pytest.param(
{
"🐉🎄 Ⓣ卄Ｉ𝐬 ⓘ𝕊 α ש𝓐𝐋𝐢𝓓 ρｌάţƑᵒᖇ𝔪 ᑎคⓜⒺ 👣♡": { # noqa: RUF001
"build-on": ["amd64"],
"build-for": ["all"],
}
},
{
"🐉🎄 Ⓣ卄Ｉ𝐬 ⓘ𝕊 α ש𝓐𝐋𝐢𝓓 ρｌάţƑᵒᖇ𝔪 ᑎคⓜⒺ 👣♡": [ # noqa: RUF001
(craft_platforms.DebianArchitecture.AMD64, "all"),
],
},
id="crazy-text",
),
pytest.param(
{
"ǝɯɐu ɯɹoɟʇɐld pılɐʌ ɐ sı sıɥ⊥": { # noqa: RUF001
"build-on": ["amd64"],
"build-for": ["all"],
}
},
{
"ǝɯɐu ɯɹoɟʇɐld pılɐʌ ɐ sı sıɥ⊥": [ # noqa: RUF001
(craft_platforms.DebianArchitecture.AMD64, "all"),
],
},
id="southern-hemisphere-validity",
),
],
)
def test_build_plans_success(
Expand Down