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

feat: add publisher gateway client #240

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

lengau
Copy link
Collaborator

@lengau lengau commented Dec 11, 2024

This adds a very basic publisher gateway client that meets the needs of charmcraft's new 'create-track' command.

The client cannot login yet and still depends on the old requests-based clients for many features.

Potentially resolves #239

CRAFT-3424

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

This adds a very basic publisher gateway client that meets the needs
of charmcraft's new 'create-track' command.

The client cannot login yet and still depends on the old requests-based
clients for many features.
@lengau lengau force-pushed the work/charmcraft-1901 branch from c84c292 to 1eb2088 Compare December 11, 2024 01:19
@lengau lengau marked this pull request as ready for review December 11, 2024 01:25
craft_store/candidauth.py Outdated Show resolved Hide resolved
craft_store/candidauth.py Outdated Show resolved Hide resolved
craft_store/candidauth.py Outdated Show resolved Hide resolved
craft_store/errors.py Show resolved Hide resolved
Retrieved from https://api.staging.charmhub.io/docs/default.html#create_tracks
"""

CreateTrackRequest = TypedDict(
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to use MarshableModel instead of TypedDict ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're trying to move away from generating and transforming the models all the time. This allows us to return the JSON dictionary returned by the store but still provide users with helpful details and linting during development.

At runtime, these are a no-op, but during development they offer both this:

Screenshot of kate with the pyright LSP completing possible package metadata keys starting with a t

and this:

image

Copy link
Contributor

@dariuszd21 dariuszd21 Dec 12, 2024

Choose a reason for hiding this comment

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

I won't block you from delivering that, but for me it looks terrible and is really bug-prone - no validation at runtime unless you explicitly do it by yourself. No control over values returned from the store and

At least we should use a dataclass-like regular syntax to make it somehow readable.
https://docs.python.org/3.12/library/typing.html#typing.TypedDict

I think at some point, we should also enforce using TypeAdapters for validating all the data that gets in and out:
https://docs.pydantic.dev/latest/concepts/type_adapter/#__tabbed_1_2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was my understanding of what @sergiusens intended - for us to be a pretty slim wrapper around the actual API. I think his intention was for us to pass the data structures as-is so that we don't need to update craft-store to be able to use additional properties that get added on the store side (either in the input or in the output).

tests/integration/publishergateway/test_read.py Outdated Show resolved Hide resolved
tests/integration/publishergateway/test_write.py Outdated Show resolved Hide resolved
craft_store/publishergateway/_publishergw.py Outdated Show resolved Hide resolved
craft_store/publishergateway/_publishergw.py Outdated Show resolved Hide resolved
tests/unit/test_publishergateway.py Outdated Show resolved Hide resolved
@lengau lengau force-pushed the work/charmcraft-1901 branch from 94f002c to 76986c4 Compare December 12, 2024 17:42
@lengau lengau requested a review from dariuszd21 December 12, 2024 17:43
@lengau lengau force-pushed the work/charmcraft-1901 branch from 76986c4 to f06d037 Compare December 12, 2024 17:45
@lengau lengau force-pushed the work/charmcraft-1901 branch from f06d037 to 771e63c Compare December 12, 2024 17:46
Copy link
Contributor

@dariuszd21 dariuszd21 left a comment

Choose a reason for hiding this comment

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

Looks good for me!

tests/integration/publisher/test_read.py Show resolved Hide resolved
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.

create a client for the publisher gateway
2 participants