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

Set a dummy-version if none set, and remove unused APT_MIRROR build-arg #3267

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

thaJeztah
Copy link
Member

Set a dummy-version if none set

Make sure the Dockerfiles can be built even if no VERSION build-arg is passed.

Makefile: remove unused APT_MIRROR build-arg

The APT_MIRROR build-arg was removed from the Dockerfile in commit
ee23105, but wasn't removed from the
Makefile.

@thaJeztah thaJeztah self-assigned this May 28, 2024
@thaJeztah thaJeztah changed the title Add default version Set a dummy-version if none set, and remove unused APT_MIRROR build-arg May 28, 2024
Dockerfile Outdated
@@ -6,7 +6,7 @@ FROM python:${PYTHON_VERSION}
WORKDIR /src
COPY . .

ARG VERSION
ARG VERSION=0.0.0-dev
Copy link
Member Author

Choose a reason for hiding this comment

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

I chose 0.0.0-dev, but not sure if anything uses the version, so if 99.9.9-dev or 99.0.0-dev ("always higher than what exists") would be better.

Happy to change!

Copy link
Member

Choose a reason for hiding this comment

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

Okay either way, but maybe someone with more python experience than me can chime in.

Copy link
Member Author

Choose a reason for hiding this comment

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

@neersighted any preference here?

Copy link
Contributor

Choose a reason for hiding this comment

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

PEP-440 defines the syntax for declaring a development package as the .devN suffix (where N is just a number), so I'd use that instead of -dev.

pip should only install stable packages when dependencies are declared as someDep >= 1.2.3, but it will allow installation of .devN dependencies if by chance the user has a dependency declared as someDep >= 4.5.6.dev2 for example.

Because of ☝️, I'd suggest staying with the lower v 0.0.0.dev just to be on the safer side in case we accidentally end up publishing a package with that version to pypi.

I'm not 100% sure this is the best approach though, so if others have more experience please chime in 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

If it expects a number after .dev, I can make it 0.0.0.dev0 I guess @neersighted SGTY?

Copy link
Member

Choose a reason for hiding this comment

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

0.0.0.dev0 is PEP 440 (though the current spec lives at https://packaging.python.org/en/latest/specifications/version-specifiers/#version-specifiers) compliant, so that works for me.

Notably all numeric components are allowed to be 0; the proposed version does pass Poetry's PEP 440 validation, which is notoriously a bit strict.

@thaJeztah thaJeztah requested review from krissetto and laurazard May 28, 2024 09:47
@thaJeztah thaJeztah requested a review from neersighted May 28, 2024 11:41
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Use a PEP 440-compliant version (e.g. the proposed 0.0.0.dev0 works).

@thaJeztah
Copy link
Member Author

@neersighted updated to use 0.0.0.dev0 👍

Make sure the Dockerfiles can be built even if no VERSION build-arg
is passed.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The APT_MIRROR build-arg was removed from the Dockerfile in commit
ee23105, but wasn't removed from the
Makefile.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@neersighted neersighted merged commit fba6ffe into docker:main Nov 18, 2024
11 checks passed
@thaJeztah thaJeztah deleted the add_default_version branch November 18, 2024 15:06
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.

4 participants