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

Add rm context command #30

Merged
merged 5 commits into from
Jun 3, 2022

Conversation

jessicalins
Copy link
Contributor

I thought would be nice to have a command that removes the context, without the need to individually remove tenant and/or api.
This PR adds the following behavior:

  • obsctl context rm <api>/<tenant> is the command
  • It removes a tenant from an api. If the api has only one tenant, the whole api is removed
  • It sees context as the relation between api+tenant. If an api does not have a tenant, the rm command cannot be used. The command obsctl context api rm <api> can be used though.

If we agree on this behavior, I'd then add tests + update docs :)
It partially implements #23

Signed-off-by: Jéssica Lins [email protected]

Signed-off-by: Jéssica Lins <[email protected]>
Signed-off-by: Jéssica Lins <[email protected]>
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks! This behavior makes sense to me! 🙂

Adding test + docs would be cool!

@douglascamata
Copy link

It removes a tenant from an api. If the api has only one tenant, the whole api is removed

I think there has to be some extra care when writing documentation and talking about this. With this sentence, some may understand that there is an API in Observatorium to manage tenants and that obsctl can interact with it to add/remove tenants... which is not the case.

Signed-off-by: Jéssica Lins <[email protected]>
Signed-off-by: Jéssica Lins <[email protected]>
@jessicalins jessicalins marked this pull request as ready for review June 2, 2022 14:02
@jessicalins
Copy link
Contributor Author

@saswatamcode @douglascamata thanks for the review! I've added test + modified the docs. I think we can improve docs in another pr (related issue: #28)

Copy link
Contributor

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Looking good 👓

README.md Outdated Show resolved Hide resolved
Co-authored-by: Matej Gera <[email protected]>
@matej-g matej-g merged commit 6ed7af6 into observatorium:main Jun 3, 2022
@jessicalins jessicalins deleted the extend-context-command branch June 3, 2022 11:49
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.

4 participants