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

docs: add renku session RFC #2687

Merged
merged 6 commits into from
Mar 7, 2022
Merged

docs: add renku session RFC #2687

merged 6 commits into from
Mar 7, 2022

Conversation

vigsterkr
Copy link
Contributor

Description

add the renku session sub-command RFC

olevski
olevski previously approved these changes Feb 15, 2022
Copy link
Member

@olevski olevski 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 to me. Are the last two sections meant to be blank - ie just have questions with no answers?

Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

this is great, thanks @vigsterkr for separating it out from the implementation. I still don't fully understand how you intend to find the sessions in commands that require a session ID without knowing which provider to query. I guess it might not matter much in practice, but it seems unnecessary to fetch all lists of sessions from all providers every time. Either we should encode the provider name in the session id, e.g.

renkulab.io/<id>
docker/<id>

or use a --provider flag in those commands as well. I prefer the first option because it's less typing for the user... but we have to come up with a good scheme.

@vigsterkr
Copy link
Contributor Author

vigsterkr commented Feb 20, 2022

this is great, thanks @vigsterkr for separating it out from the implementation. I still don't fully understand how you intend to find the sessions in commands that require a session ID without knowing which provider to query. I guess it might not matter much in practice, but it seems unnecessary to fetch all lists of sessions from all providers every time. Either we should encode the provider name in the session id, e.g.

renkulab.io/<id> docker/<id>

or use a --provider flag in those commands as well. I prefer the first option because it's less typing for the user... but we have to come up with a good scheme.

the primary reason was to avoid the explicit mentioning of the provider in some cases (see when dealing with session_id) in order to avoid unnecessary typing on the CLI by the user. pluggy by default calls all the registered plugin hooks, but of course there's a way around this behaviour... but i thought that the overhead on the backend side is less than the gained value of not typing things out. This week we talked out prefixing the session_id (mentioned above) with the provider, but to be honest then this adds additional string processing (prone to errors) on the backend side. in this case i'd rather just go with the explicit requiring the -p/--provider <provider> flag from the user. coz essentially it'll do the same thing on the end of the day (from pluggy perspective), but we'd just use the user provided provider instead of doing some string processing for extracting the provider type and the session_id

@rokroskar
Copy link
Member

in this case i'd rather just go with the explicit requiring the -p/--provider flag from the user

That's fine - the default can be to cycle through all of them but having a consistent interface where one can specify a provider makes sense to me.

@vigsterkr vigsterkr requested a review from rokroskar March 4, 2022 12:18
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

two small comments but then I think we're finally done :)

@vigsterkr vigsterkr enabled auto-merge (squash) March 5, 2022 13:12
@vigsterkr vigsterkr merged commit ac6c7bc into develop Mar 7, 2022
@vigsterkr vigsterkr deleted the feature/session-rfc branch March 7, 2022 08:37
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.

None yet

4 participants