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: add workspace subcommands #148

Merged
merged 17 commits into from
Oct 25, 2024

Conversation

dtdang
Copy link
Contributor

@dtdang dtdang commented Oct 21, 2024

What I did

Add workspaces subcommands to cli

fixes: #141

How I did it

How to verify it

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
@dtdang dtdang marked this pull request as ready for review October 23, 2024 16:47
silverback/_cli.py Show resolved Hide resolved
silverback/_cli.py Show resolved Hide resolved
silverback/_cli.py Show resolved Hide resolved
fubuloubu
fubuloubu previously approved these changes Oct 25, 2024
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
Comment on lines 326 to 327
update_name: str,
update_slug: str,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
update_name: str,
update_slug: str,
update_name: str | None,
update_slug: str | None,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user doesn't have an update name or slug, it seems like it won't keep the previous name/slug. At least that was the case when I tried it using the /docs/ page, or was that a bug?

Copy link
Member

Choose a reason for hiding this comment

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

so if both are empty, it should raise UsageError because you haven't tried to update anything

are you saying if one is empty and the other is intended to be changed, they both update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally it was if one is empty and the other is intended then they both updated. The new push I made should have fixed that issue though I think.

Copy link
Member

Choose a reason for hiding this comment

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

the service itself should allow None values and basically make it "no change required", so you shouldn't have to set the values if they are not set and it should work appropiately

my point above is that if both are None that is a no-op (won't make any change when you send the PATCH request to the service) so it should raise click.UsageError that it's a no-op

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to update just one value of both name and slug but if a value is left as None then the value will be patched in the workspace as ''.

silverback cluster workspaces info -p staging testing3

Name:               # should be testing2 since value was left as None
Slug: 'testing3'
Date Created: '2024-10-23 15:25:48.249070+00:00'

Added the click.UsageError if both values are None.

fubuloubu
fubuloubu previously approved these changes Oct 25, 2024
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Some small nits, lfg

silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/cluster/client.py Outdated Show resolved Hide resolved
silverback/cluster/client.py Outdated Show resolved Hide resolved
@dtdang dtdang enabled auto-merge (squash) October 25, 2024 16:38
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Test all the commands one last time and then we can release this

@dtdang dtdang merged commit d127f7d into ApeWorX:main Oct 25, 2024
13 checks passed
@dtdang
Copy link
Contributor Author

dtdang commented Oct 25, 2024

Test all the commands one last time and then we can release this

All the commands work. The only thing of concern is that when updating only one value of either the name or slug, the value that isn't updated is patched as an empty string. This is the same behavior when done directly through the fastapi /docs page so I believe that it might be a bug with the platform.

$ silverback cluster workspaces update -p staging finalcheck -n updatecheck

SUCCESS: Updated 'updatecheck'

$ silverback cluster workspaces info -p staging ''

Name: updatecheck
Slug: ''
Date Created: '2024-10-25 17:23:39.367515+00:00'

@dtdang dtdang deleted the feat/workspaces_subcommands branch October 25, 2024 20:25
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.

Add full silverback cluster workspaces subcommand group (not including member management, separate issue)
2 participants