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

sketching API v2 #1048

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

sketching API v2 #1048

wants to merge 10 commits into from

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 4, 2020

The event stream errors return, which prompt me to want to take a stab at the proposal in #358, which should significantly reduce the consequences of event stream issues (in theory, to zero).

The gist of the design from #358, by @yuvipanda:

  • POST to start build, replies with URL to watch events
  • event stream requests only get logs, never result in transactions
  • POST to launch
  • opaque JWT tokens used to pass stateful args between stages (build id, hub username, token, etc.)

Python code design choices so far:

  • create v1, v2 subfolders for API handlers
  • split builder.py into a Builder class which implements building and a v1 BuildHandler. The Builder will be used in both v1 and v2
  • create base EventStreamHandler class, used in both v1 and v2

My plan is to refactor things based on v2 plans, while keeping v1 fully working

implementation log

This is a very early WIP. I'll update with progress notes.

implementation challenges: lots of coupling between different layers that make separating these actions tricky, plus preserving the old handlers without tons of duplicate code. Separating them will be helpful in the long run, but will take some doing.

Couplings found so far:

  • build/watch/launch logic is all in BuildHandler, much of which will need to be re-used in both v1 and v2 handlers (Builder object should help with this)
  • Start-build and watch-build are tied together in the Build object, when ~only the build-id and namespace are required for watching builds.

closes #358
closes #304
closes #13
closes #15

@betatim
Copy link
Member

betatim commented Feb 5, 2020

Maybe off topic but seems worth pondering while working on this: #1038 to reduce the amount of duplicate information in our code base (each builder has to be defined in Python code and again in JS).

Another new endpoint we have discussed is one that would implement the logic needed for an "awesome bar". In my mind that is an endpoint to which you POST "what ever a user typed into the awesome bar" and get back structured information which can then be used to trigger a build.

I don't think these two points are directly relevant for this but might be worth keeping at the back of our minds so we can make it easier (in the future) to build them (vs harder because we made choices here without considering them).

@minrk
Copy link
Member Author

minrk commented Feb 13, 2020

Definitely worth keeping in mind! I think both of those issues are a bit orthogonal to this change, though. Awesome bar, for example, would be a step 0 request, redirecting to the appropriate, stricter, versioned build API URL. And the server-side provider info would also happen at the same level as in #1038, I don't expect this change should affect that effort. Good to have thought that through, though.

@consideRatio consideRatio added code:python Python changes. new labels Oct 10, 2021
@consideRatio consideRatio marked this pull request as draft December 29, 2022 13:58
@consideRatio consideRatio changed the title [WIP] sketching API v2 sketching API v2 Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code:python Python changes. new
Projects
None yet
3 participants