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: update and expand apispec for environments #300

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

Conversation

olevski
Copy link
Member

@olevski olevski commented Jul 10, 2024

Still WIP.

@olevski olevski requested a review from a team as a code owner July 10, 2024 15:15
@olevski olevski marked this pull request as draft July 10, 2024 15:15
@olevski olevski requested a review from andre-code July 10, 2024 15:16
@andre-code
Copy link
Contributor

Thanks, @olevski , for putting this together. I have a few observations:

  1. Does the /Environment (GET) endpoint only return global environments? If not, we might need a parameter to filter them.
  2. Is it necessary to add the new parameters like working_directory and port to the EnvironmentPost object? So far, that endpoint is for adding global environments, and since it's not part of this build's scope, I'd suggest not including them yet.
  3. I noticed there's now a visibility property for an environment. How is the UI supposed to handle this? Just display it?

@olevski
Copy link
Member Author

olevski commented Jul 11, 2024

Does the /Environment (GET) endpoint only return global environments? If not, we might need a parameter to filter them.

This returns all environments you can filter for the global environments by owner_type="platform".

Is it necessary to add the new parameters like working_directory and port to the EnvironmentPost object? So far, that endpoint is for adding global environments, and since it's not part of this build's scope, I'd suggest not including them yet.

That endpoint should be used to add all environments. But since those things are not required you can leave them undefined and the defaults will be used. Or we implement this but we disable adding global environments. I am not sure why this is out of the scope here? The scope is to enable to specify all these properties on environments. And to me this means all environments, regardless of whether they are global or not.

I noticed there's now a visibility property for an environment. How is the UI supposed to handle this? Just display it?

Yes just display it.

@olevski
Copy link
Member Author

olevski commented Jul 11, 2024

For the global environments you can even fully ignore these additional fields. Ignore them when you create them and also ignore them when you list them. At least for the implementation in this pitch.

@leafty
Copy link
Member

leafty commented Jul 12, 2024

@olevski the idea here is that orthogonal changes in the same PR is not best practice. Changes not required to power non-jupyter images should be slated for another PR. Also, to avoid changing things multiple times, we should wait until namespaced environments are shaped to include ownership because that notion will not exist until then.

@olevski
Copy link
Member Author

olevski commented Jul 12, 2024

@leafty when I wrote this it was compatible with the vision for sharing. But then that changed yesterday by the end of the day so now it is not.

Changes not required to power non-jupyter images should be slated for another PR

But these changes are specifically meant for non-jupyter images. Both the spec for the global environments and the spec for the custom environments have to change to accomodate non-jupyter images. And the implementation of this on the backend is really strange so I really do not want to keep adding things in the current organization of the backend. I suppose these are the orthogonal changes you are referring to? The problem is that the backend has one way for storing the global environments and a completely different one for storing the custom ones in a session launcher. But they should be uniform.

I will change this further to be more in line with what we discussed about sharing yesterday. And I will try to hide some of the sharing related concepts from the API.

@olevski olevski force-pushed the feat-add-new-environments-apispec branch from 00ef3ba to 86b8962 Compare July 12, 2024 08:57
@olevski
Copy link
Member Author

olevski commented Jul 12, 2024

Ok so these are the changes I made:

  • /environments/* endpoints only deal with global environments same as before, as such the request and response formats do not include any of the fields about owner_type, owner_id
  • owner_type and owner_id are fully removed from the spec
  • environment_kind was put back in and is used only in the requests/responses from session_launchers/* endpoints - this is same as before (i.e. what is currently on dev.renku.ch)
  • visibility field has been removed from all responses

So this pretty much fully sidesteps the issue of sharing.

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.

3 participants