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

refactor(breadbox): remove & auto-generate breadbox client #119

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

snwessel
Copy link
Member

@snwessel snwessel commented Nov 4, 2024

These changes include:

  1. Ignoring the auto-generated breadbox-client/breadbox_client directory (so they're no longer checked into version control)
  2. Setting up an action to auto-generate the breadbox client. This is used in two separate github workflows:
    a. pyright-breadbox-client: validates that the breadbox-facade types are valid with the auto-generated client
    b. publish-breadbox-client: publishes the breadbox-client module (including both the auto-generated code and the facade)
  3. Updating the instructions for local development

I also had a couple of questions that I thought might be worth discussing. I added them to our breadbox meeting list here:

  • I'm wondering how people would feel about removing the breadbox-client-generator repo. I had an easier time setting up the github action without it (by using pip to just install the one package)
  • I'm also wondering why the breadbox module was being published as a python module, and whether we want to continue doing that? I have removed that step for now.

I'll wait to merge these changes until after we talk through the open questions

@snwessel snwessel changed the title Remove autogen bb client refactor(breadbox): autogen bb client Nov 4, 2024
@snwessel snwessel changed the title refactor(breadbox): autogen bb client refactor(breadbox): remove & auto-generate breadbox client Nov 4, 2024
@snwessel snwessel marked this pull request as ready for review November 5, 2024 14:33
In general, the `breadbox_client` directory is auto-generated in a github action. It is not checked in to the repo.

If you would like to generate the client for local developmen/debugging, follow these steps:
1. Install the `openapi-python-client` with the python environment manager of your choice. For example: `pipx install openapi-python-client==0.21.1`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why run this long command instead of ./bb update-client like we've been doing to date?

If I'm making changes to breadbox and want to test the client in dev to evaluate them, I'd want to get a client using a newly generated api.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of the things I wanted to get input on, but after thinking about it a bit more, I think you're right that the best option is to keep using the ./bb update-client command for local development.

Historically, I've had some issues using that command without doing all of the following steps (and I wanted to ask if others have had similar issues):

    cd breadbox-client-generator/
    poetry install && poetry shell
    cd ../breadbox
    ./bb update-client

However, I suspect that might have actually been an issue with my local python/poetry configuration, because the ./bb update-client command does seem to be working for me now (and I'm not sure what changed).

I am going to undo some of the changes I had made on this branch so we can keep using that command 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, I made the couple of changes we talked about. I also added a note to our check-in list so we remember to follow-up on potentially getting the ./bb update-client command working in github actions. I'll wait to make any other changes to this branch until after we talk about that.

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.

2 participants