search

vapor-ware / synse-cli

Unified CLI for the Synse platform
GNU General Public License v3.0
1 stars 4 forks source link

feat: add unset context command #244

Closed nideshchitrakar closed 1 year ago

nideshchitrakar commented 1 year ago

This PR:

Question - should an error be raised if the current context is not the one provided?

VIO-3177

DillonEnge commented 1 year ago

@nideshchitrakar Will we ever have a scenario in which we would have to explicitly provide a context given that only one can be active at any given time? This feels more appropriate to ditch the name argument and just have it unset the current if possible/preferable.

nideshchitrakar commented 1 year ago

@nideshchitrakar Will we ever have a scenario in which we would have to explicitly provide a context given that only one can be active at any given time? This feels more appropriate to ditch the name argument and just have it unset the current if possible/preferable.

That's a good call out. I added the context argument because of this ticket description:

Unset will remove the entry as a current context. It should return an error if the context does not exist.

I wasn't sure if it meant if the user needed to provide a context name, or if the error should be raised if there is no context available at all.

nideshchitrakar commented 1 year ago

@DillonEnge actually circling back to contexts, we can have two contexts active at a time, one each for a server and a plugin instance. So there needs to be a way to specify which one to target, whether by passing a context name or a context type.

DillonEnge commented 1 year ago

@DillonEnge actually circling back to contexts, we can have two contexts active at a time, one each for a server and a plugin instance. So there needs to be a way to specify which one to target, whether by passing a context name or a context type.

I think in my mind that would make more sense for that to be the switched on value rather than name so my opinionated suggestion is to target a TYPE rather than a NAME. Feel free to go that route or keep it as is, I will slap an approve on this as more up to preference. Concerning the warning I do think it would be ideal to flash an error if there isn't a currently set context of the given type if we go that route.

nideshchitrakar commented 1 year ago

Yeah that totally makes synse. I'll wait for @GuessWhoSamFoo 's thoughts before I potentially make this change.