Skip to content

Commit

Permalink
refactor: updates to use display name
Browse files Browse the repository at this point in the history
  • Loading branch information
JGSweets committed Jul 4, 2024
1 parent 0af7e9a commit b925cf1
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 21 deletions.
30 changes: 15 additions & 15 deletions sky/clouds/aws.py
Original file line number Diff line number Diff line change
Expand Up @@ -399,12 +399,15 @@ def make_deploy_resources_variables(
image_id = self._get_image_id(image_id_to_use, region_name,
r.instance_type)

user_security_group = skypilot_config.get_nested(

user_security_group_config = skypilot_config.get_nested(
('aws', 'security_group_name'), None)
if user_security_group is not None and not isinstance(
user_security_group, str):
for profile in user_security_group:
if fnmatch.fnmatchcase(cluster_name.name_on_cloud,
user_security_group = None
if isinstance(user_security_group_config, str):
user_security_group = user_security_group_config
elif isinstance(user_security_group_config, list):
for profile in user_security_group_config:
if fnmatch.fnmatchcase(cluster_name.display_name,
list(profile.keys())[0]):
user_security_group = list(profile.values())[0]
break
Expand All @@ -414,18 +417,15 @@ def make_deploy_resources_variables(
if resources.ports is not None:
# Already checked in Resources._try_validate_ports
security_group = USER_PORTS_SECURITY_GROUP_NAME.format(
cluster_name.name_on_cloud)
elif security_group is not None and resources.ports is not None:
cluster_name.display_name)
elif resources.ports is not None:
with ux_utils.print_exception_no_traceback():
logger.warning(
f'Ports {resources.ports} and security group name are '
f'specified: {security_group}. It is not '
'guaranteed that the ports will be opened if the '
'specified security group is not correctly set up. '
'Please try to specify `ports` only and leave out '
'`aws.security_group_name` in `~/.sky/config.yaml` to '
'allow SkyPilot to automatically create and configure '
'the security group.')
f'Skip opening ports {resources.ports} for cluster {cluster_name!r}, '
'as `aws.security_group_name` in `~/.sky/config.yaml` is specified as '
f' {security_group!r}. Please make sure the specified security group '
'has requested ports setup; or, leave out `aws.security_group_name` '
'in `~/.sky/config.yaml`.')

return {
'instance_type': r.instance_type,
Expand Down
2 changes: 0 additions & 2 deletions tests/test_yamls/test_aws_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ aws:
remote_identity:
- sky-serve-fake1-*: fake1-skypilot-role
- sky-serve-fake2-*: fake1-skypilot-role
- "*": fake-skypilot-default-role

security_group_name:
- sky-serve-fake1-*: fake-1-sg
- sky-serve-fake2-*: fake-2-sg
- "*": fake-skypilot-default
9 changes: 5 additions & 4 deletions tests/unit_tests/test_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def test_kubernetes_labels_resources():
return_value={"fake-acc": 2})
@patch("sky.clouds.service_catalog.get_image_id_from_tag",
return_value="fake-image")
@patch.object(clouds.aws, "DEFAULT_SECURITY_GROUP_NAME", "fake-default-sg")
def test_aws_make_deploy_variables(*mocks) -> None:
skypilot_config._try_load_config()

Expand Down Expand Up @@ -133,15 +134,15 @@ def test_aws_make_deploy_variables(*mocks) -> None:
# test using defaults
expected_config = expected_config_base.copy()
expected_config.update({
'security_group': "fake-skypilot-default",
'security_group_managed_by_skypilot': 'false'
'security_group': "fake-default-sg",
'security_group_managed_by_skypilot': 'true'
})
assert config == expected_config, ('unexpected resource '
'variables generated')

# test using culuster matches regex, top
cluster_name = resources_utils.ClusterName(
display_name='display', name_on_cloud='sky-serve-fake1-1234')
display_name='sky-serve-fake1-1234', name_on_cloud='name-on-cloud')
expected_config = expected_config_base.copy()
expected_config.update({
'security_group': "fake-1-sg",
Expand All @@ -156,7 +157,7 @@ def test_aws_make_deploy_variables(*mocks) -> None:

# test using culuster matches regex, middle
cluster_name = resources_utils.ClusterName(
display_name='display', name_on_cloud='sky-serve-fake2-1234')
display_name='sky-serve-fake2-1234', name_on_cloud='name-on-cloud')
expected_config = expected_config_base.copy()
expected_config.update({
'security_group': "fake-2-sg",
Expand Down

0 comments on commit b925cf1

Please sign in to comment.