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

refactor: change services/default from string to list format #155

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

iprasannamb
Copy link

Change services/default from string to list format

This pull request changes the services/default configuration from a comma-separated string format to a list of strings format, making the code more robust and easier to use.

Resolves: #153

Changes

  • Updated schema.json to define 'default' as an array of strings instead of a string
  • Modified SugarBase methods in base.py to work with lists instead of comma-separated strings:
    • _load_root_services()
    • _filter_service_profile()
    • _get_services_names()
  • Updated documentation (README.md) to reflect the new list format in examples
  • Updated test configuration file to use the new list format

Before

profiles:
  profile1:
    services:
      default: service1,service3

After

profiles:
  profile1:
    services:
      default: 
        - service1
        - service3

Backward Compatibility

Backward compatibility has been maintained by automatically detecting and converting string formats to lists when encountered in existing configurations. This ensures that existing configurations with comma-separated strings will continue to work during the transition period.

Testing

The implementation has been tested with both the new list format and the legacy string format to ensure backward compatibility.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @iprasannamb! This PR contains changes from #154 too. Could you make this branch from main and not from your index branch? Rebasing on top of main would help!

Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

sorry ... I reviewed a few days ago .. but forgot to submit my review ..

@@ -123,8 +125,8 @@ Some examples of how to use it:
`sugar ext restart --profile profile1 --all`

- restart service1 and service2 for profile1:
`sugar ext restart --profile profile1 --services service1,service2`
`sugar ext restart --profile profile1 --services service1 service2`
Copy link
Member

Choose a reason for hiding this comment

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

I guess that the cli will not change

@@ -1,127 +0,0 @@
# Get Started
Copy link
Member

Choose a reason for hiding this comment

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

it seems that this change is the same as your previous PR
please ensure every time you start a new branch from upstream/main

take a look into this blog post for more information about our workflow with github https://opensciencelabs.org/blog/first-time-contributors/

@@ -216,13 +216,18 @@ def _load_root_services(self) -> None:
if not services:
return

# Handle the case where 'default' comes as a string (for backward compatibility)
default_services = services.get('default', [])
if isinstance(default_services, str) and default_services:
Copy link
Member

Choose a reason for hiding this comment

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

although the backward compatibility is a nice thing, but in the schema.json you changed from string to array ... so this value will never be a string, so this code will not be necessary

else:
# Handle the case where 'default' comes as a string (for backward compatibility)
default_services = profile_data.get('services', {}).get('default', [])
if isinstance(default_services, str) and default_services:
Copy link
Member

Choose a reason for hiding this comment

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

same here, the default_services will be always a list

services_default = services_config.get('default', [])

# Handle the case where 'default' comes as a string (for backward compatibility)
if isinstance(services_default, str) and services_default:
Copy link
Member

Choose a reason for hiding this comment

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

same here

@xmnlab
Copy link
Member

xmnlab commented Mar 12, 2025

@iprasannamb , are you still working on that?

@xmnlab xmnlab marked this pull request as draft March 12, 2025 23:22
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.

Change services/default from string to list format
3 participants