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

Env config #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Env config #40

wants to merge 1 commit into from

Conversation

amdfxlucas
Copy link
Collaborator

@amdfxlucas amdfxlucas commented Feb 8, 2025

for description see: #5

Its a first working draft and request for comments . Just compile i.e. ScionBandwidthTester and note the '.env' file which is generated next to docker-compose.yml and the new 'environment:' sections for each service in the compose file.
You can change any of the ENV variables and simply 'docker compose stop / start' to run the simulation with the new settings, rather than recompiling & rebuilding any images


This change is Reviewable

@amdfxlucas amdfxlucas added the enhancement New feature or request label Feb 8, 2025
@amdfxlucas amdfxlucas added this to the Configurable SCION milestone Feb 8, 2025
@amdfxlucas amdfxlucas requested a review from lschulz February 8, 2025 17:05
@amdfxlucas amdfxlucas self-assigned this Feb 8, 2025
Copy link
Member

@lschulz lschulz left a comment

Choose a reason for hiding this comment

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

Works as you describe it on my system.

Have you thought about feature dependencies? For example, SCMP authentication and ID-INT require DRKey.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @amdfxlucas)


seedemu/compiler/Docker.py line 418 at r1 (raw file):

            @brief retuns ENV variables defined in the given scope
        """
        import toolz

Is this dependency worth it? (also why not import at the top of the file?) The expression below isn't very readable anyway.


seedemu/compiler/Docker.py line 434 at r1 (raw file):

        """
                
        from cawdrey import frozendict

Is there a good reason to import this type? Wouldn't a normal dict do the job?


seedemu/core/AutonomousSystem.py line 48 at r1 (raw file):

        self.__as_features = {}

    def setFeatures(self, key_val: Optional[Union[Dict[str, str], tuple]] = None, key: Optional[str] = None, val: Optional[str] = None):

Have you considered taking keyword arguments (**kwargs), so you could do something like setFeatures(bfd=False)?

@amdfxlucas
Copy link
Collaborator Author

Works as you describe it on my system.

Have you thought about feature dependencies? For example, SCMP authentication and ID-INT require DRKey.

Yes, with the reification of Options we'd have a good starting point to cater for mutual interdependencies or exclusion ...

@amdfxlucas amdfxlucas marked this pull request as ready for review February 13, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants