Add support for creating Cargo repositories

uschi2000 commented 2 years ago

Description

Type of change

Please delete options that are not relevant.

[x] New feature (non-breaking change which adds functionality)

How has it been tested ?

Verified manually against a real Artifactory instance that creates a Cargo local repository
No other tests were added or changed in accordance with the practices in this repo: I couldn't find tests that exercise specific package types.

Checklist:

[x] My PR is ready for prime time! Otherwise use the "Draft PR" feature
[x] All commits have a correct title
[ ] Readme has been updated
[x] Quality tests are green (see Codacy)
[x] Automated tests are green (see pipeline)

uschi2000 commented 2 years ago

Take a look please, @anancarv

uschi2000 commented 2 years ago

According to https://www.jfrog.com/confluence/display/JFROG/Repository+Configuration+JSON, 'Cargo' is not a supported package type for virtual repositories. As it stands, I suspect this would result in an HTTP-level error. Do you think that's OK?

anancarv commented 2 years ago

According to https://www.jfrog.com/confluence/display/JFROG/Repository+Configuration+JSON, 'Cargo' is not a supported package type for virtual repositories. As it stands, I suspect this would result in an HTTP-level error. Do you think that's OK?

Hi @uschi2000 , sorry for the late answer. As you highlighted it above, this implementation will lead to errors for virtual repositories. To overcome it, you have multiple solutions:

First, within the create and update repository methods, after having verified the type of the repository instance (VirtualRepository in your case), if the repository PackageTypeEnum is cargo raise an error.

Second, in the models/repository.py create a new VirtualRepoPackageTypeEnum that will inherit from the old one BasePackageTypeEnum. Then, VirtualRepoPackageTypeEnum will be used for creating virtual repositories and the other for local and remote.

I have a preference for the second solution

uschi2000 commented 2 years ago

I don't think we can subclass enums: https://docs.python.org/3/library/enum.html#restricted-enum-subclassing

So the next-best thing would be sth like:

class BasePackageTypeEnum(str, Enum):
    """Enumerates the package types that all repository types support."""

    # Keep these in sync with ExtendedPackageTypeEnum
    maven = "maven"
    gradle = "gradle"
    ivy = "ivy"
    sbt = "sbt"
    helm = "helm"
    cocoapods = "cocoapods"
    opkg = "opkg"
    rpm = "rpm"
    nuget = "nuget"
    cran = "cran"
    gems = "gems"
    npm = "npm"
    bower = "bower"
    debian = "debian"
    pypi = "pypi"
    docker = "docker"
    yum = "yum"
    vcs = "vcs"
    composer = "composer"
    go = "go"
    p2 = "p2"
    chef = "chef"
    puppet = "puppet"
    generic = "generic"
    conan = "conan"
    alpine = "alpine"
    gitlfs = "gitlfs"
    vagrant = "vagrant"
    conda = "conda"

class ExtendedPackageTypeEnum(str, Enum):
    """Enumerates the additional package types supported by some repository types."""

    # Keep these in sync with BasePackageTypeEnum
    maven = "maven"
    gradle = "gradle"
    ivy = "ivy"
    sbt = "sbt"
    helm = "helm"
    cocoapods = "cocoapods"
    opkg = "opkg"
    rpm = "rpm"
    nuget = "nuget"
    cran = "cran"
    gems = "gems"
    npm = "npm"
    bower = "bower"
    debian = "debian"
    pypi = "pypi"
    docker = "docker"
    yum = "yum"
    vcs = "vcs"
    composer = "composer"
    go = "go"
    p2 = "p2"
    chef = "chef"
    puppet = "puppet"
    generic = "generic"
    conan = "conan"
    alpine = "alpine"
    gitlfs = "gitlfs"
    vagrant = "vagrant"
    conda = "conda"

    # Additional package types below
    cargo = "cargo"

Alternatively, we could turn each of these enum variants into a class and then define a new type as

BasePackageType = Union[MavenPackageType, ..., CondaPackageType]
ExtendedPackageType = Union[MavenPackageType, ..., CondaPackageType, CargoPackageType]

Or, back to the drawing board, we can make this a runtime rather than compile-time concern.

What do you think, @anancarv ?

uschi2000 commented 2 years ago

Ping, @anancarv . Any opinions on this?

anancarv commented 2 years ago

Hi @uschi2000 , After having thought about it, I think we can accept your first contribution. Indeed, adding new enums will increase the code duplication and it can be hard to maintain afterwards. I'll just need to check what http error is thrown by the user trying to create a Cargo virtualRepository to see if it's understandable by the user making the request

uschi2000 commented 2 years ago

Sounds good, @anancarv . I'll run this against a real artifactory instance to check what the error is.

nymous commented 2 years ago

I tested on our SaaS Artifactory instance, and... no error is returned from the API :sweat_smile: With the following code

art = Artifactory("https://artifactory.example", ("username", "api key"))

repo = VirtualRepository(key="test-nymous-cargo", packageType=PackageTypeEnum.cargo)

print(repo)
print(art.repositories.create_repo(repo))

The code doesn't error, the API is happy, the repository is created and visible in the "Virtual repositories" list... But if you try to click on it to edit its configuration the UI goes "Nope, error 500".

https://user-images.githubusercontent.com/4216559/147482035-674544d9-e92b-4f2e-89bb-3b005c748687.mp4

(And as a side note, the UI to create a virtual repository confirms that Cargo is not supported, as it is not listed.)

nymous commented 2 years ago

And regarding the issue of not being able to subclass enums, I have a (breaking) suggestion: how about using Literals? The IDE support is a bit poorer (I think Pycharm and VSCode don't autocomplete supported values when you call the function), but Literals are still typed and can be composed. See this simplified example:

from typing import Literal, Union

BasePackageType = Literal["maven", "gradle", "debian", "helm", "generic"]

class VirtualRepository:
    packageType: BasePackageType = "generic"

class LocalRepository:
    packageType: Union[BasePackageType, Literal["cargo"]] = "generic"

reveal_type(VirtualRepository.packageType)
# Revealed type is "Union[Literal['maven'], Literal['gradle'], Literal['debian'], Literal['helm'], Literal['generic']]"
reveal_type(LocalRepository.packageType)
# Revealed type is "Union[Literal['maven'], Literal['gradle'], Literal['debian'], Literal['helm'], Literal['generic'], Literal['cargo']]"

anancarv commented 2 years ago

Using Literal is a great feature, but will be a breaking change as you said. We can keep it as a future improvement of the package. For now, let's just accept the PR as it is.

uschi2000 commented 2 years ago

Cheers @anancarv . Could you also carve a release please?

anancarv / python-artifactory