Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

https://github.com/twosigma/waiter/issues/1649 + https://github.com/twosigma/waiter/issues/1637 #1672

Closed
wants to merge 4 commits into from
Closed

https://github.com/twosigma/waiter/issues/1649 + https://github.com/twosigma/waiter/issues/1637 #1672

wants to merge 4 commits into from

Conversation

derik01
Copy link
Collaborator

@derik01 derik01 commented Oct 22, 2022

Changes proposed in this PR

#1649
#1637

Why are we making these changes?

  • Allow all users to specify container name for SSH command
  • Add service state to show CLI command

shamsimam
shamsimam reviewed Oct 22, 2022
View reviewed changes
Copy link
Contributor

@shamsimam shamsimam left a comment

Choose a reason for hiding this comment

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

Please create separate prs for the two changes.

cli/integration/tests/waiter/test_cli.py
@@ -1154,6 +1154,7 @@ def test_show_services_using_token(self):
self.assertEqual(service_description_2, service_2['service-description'])
self.assertEqual('Running', service_1['status'])
self.assertIn(service_2['status'], ['Failing', 'Starting'])
self.assertIn(service_2['scaling-state'], ["stable", "scale-up", "scale-down"])
Copy link
Contributor

@shamsimam shamsimam Oct 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let's capitalize the first letter in the output

Actually, let it be. We can fix the string in the api if needed.

cli/waiter/display.py
@@ -65,7 +65,8 @@ def tabulate_token_services(services, token_name, token_etag=None, show_index=Fa
('Last request', format_last_request_time(s)),
('Current?',
format_using_current_token(s, token_etag or s.get('etag', None),
token_name))]
token_name)),
('Scaling State', s['scaling-state'])]
Copy link
Contributor

Choose a reason for hiding this comment

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

the code should be defensive against the value missing and display blank or Unknown. As written the code will throw an error if the scaling-state entry is missing.

@shamsimam
Copy link
Contributor

Some cli tests are failing as status is no longer the last column, the tests need to be updated:

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/waiter/test_cli_multi_cluster.py:112: in __test_show_single_cluster_group
    self.assertIsNotNone(re.search(f'^{service_id_2}\\s+waiter2[^\\n]+Running[^\\n]+Not Current$', cli.stdout(cp), re.MULTILINE))
E   AssertionError: unexpectedly None

from output:

Service Id                                                         Cluster    Run as user      Instances    CPUs  Memory    Version                               Status    Last request    Current?     Scaling State
waiter-service-serviceqwkwkl3sdf-fb1036f88c5965ab93e5cb28dfa69973  waiter2    runner                   1       1  256 MiB   636464ad-f78c-45ac-85c7-8a76ab9eed4f  Running   just now        Not Current  stable
waiter-service-servicet69za6u9wo-42130d9856885c4488e84dae2845f818  waiter1    runner                   1       1  256 MiB   c95f7f31-53d5-489a-a386-840b5af3b69f  Running   just now        Current      stable

https://github.com/twosigma/waiter/actions/runs/3301534112/jobs/5447110173

@shamsimam
Copy link
Contributor

Closing this PR since two new ones were opened:

@shamsimam shamsimam closed this Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shamsimam shamsimam shamsimam left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@derik01 @shamsimam