search

Azure / simdem

Tool for Simulating Demo's, delivering Tutorials and using documentation as tests.
MIT License
34 stars 17 forks source link

$SIMDEM_TEMP_DIR and SIMDEM_VERSION not set in SimDem2 #104

Closed SorraTheOrc closed 6 years ago

SorraTheOrc commented 6 years ago

In SimDem1 we provided default SIMDEM_TEMP_DIR environment variable, this is used as a workspace when SimDem or a SimDem script needs to write some temporary files. By default it is set to [.simdem/tmp](https://github.com/Azure/simdem/blob/6dbce90c0bfbd980e35cdc3c92edd28e1e612ffe/config.py#L2)

Similarly we set SIMDEM_VERSION for easy validation of the version against expected versions during testing. This could be replaced by a --version switch in the CLI.

SimDem2 does not provide either of these and so the test scripts for SimDem1 fail:

$ ~/.local/bin/simdem --mode test README.md
$ ls $SIMDEM_TEMP_DIR/test
ls: cannot access '/test': No such file or directory

***PREREQUISITE VALIDATION FAILED***
$ touch $SIMDEM_TEMP_DIR/test/nested_prereq_ran
touch: cannot touch '/test/nested_prereq_ran': No such file or directory

$ ls $SIMDEM_TEMP_DIR/test
ls: cannot access '/test': No such file or directory

***PREREQUISITE VALIDATION FAILED***
$ touch $SIMDEM_TEMP_DIR/test/prereq_ran
touch: cannot touch '/test/prereq_ran': No such file or directory

$ ls $SIMDEM_TEMP_DIR/test
ls: cannot access '/test': No such file or directory

***PREREQUISITE VALIDATION FAILED***
$ touch $SIMDEM_TEMP_DIR/test/nested_prereq_ran
touch: cannot touch '/test/nested_prereq_ran': No such file or directory

$ ls $SIMDEM_TEMP_DIR/test
ls: cannot access '/test': No such file or directory

***PREREQUISITE VALIDATION FAILED***
$ touch $SIMDEM_TEMP_DIR/test/prereq_ran
touch: cannot touch '/test/prereq_ran': No such file or directory

$ echo $SIMDEM_VERSION

*** SIMDEM TEST RESULT FAILED ***
lastcoolnameleft commented 6 years ago

I've added --version flag in this commit: https://github.com/Azure/simdem/commit/c36443103bbf81103b34dc3f86eba90975a93e02

➜  simdem git:(simdem2) simdem --version
simdem 0.9.0

Does this meet the $SIMDEM_VERSION requirement, or did you still want the env variable injected?

SorraTheOrc commented 6 years ago

To be honest I can't for the life of me remember why there is an environment variable for VERSION, so I guess that means we can drop it.

The TEMP_DIR is important though, many of the scripts already out there use it. Happy for this to simply be a default that can be overridden with the '-e' argument. I keep dreaming of a .simdem/config file to configure these kinds of things but we should not block on that.

lastcoolnameleft commented 6 years ago

Regarding the directory location, it's easy to put into the config file (which is overridable); however, the key part is what is it relative to?

SorraTheOrc commented 6 years ago

Current implementation is relative to home

Get Outlook for Androidhttps://aka.ms/ghei36

From: Tommy Falgout notifications@github.com Sent: Friday, March 16, 2018 11:17:01 AM To: Azure/simdem Cc: Ross Gardler; Author Subject: Re: [Azure/simdem] $SIMDEM_TEMP_DIR and SIMDEM_VERSION not set in SimDem2 (#104)

Regarding the directory location, it's easy to put into the config file (which is overridable); however, the key part is what is it relative to?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fsimdem%2Fissues%2F104%23issuecomment-373801234&data=04%7C01%7Cross.gardler%40microsoft.com%7Cf8dd4874546647d96b7e08d58b6a1e6f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636568210252407254%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=KGVbvt85WJDhpCivM2qIopK6x7%2B8OJhaBMe98ImnUk8%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAPRgAIZ62Is-3kK6hlQHlh_Mz0pbs9nks5tfAGdgaJpZM4Sm98E&data=04%7C01%7Cross.gardler%40microsoft.com%7Cf8dd4874546647d96b7e08d58b6a1e6f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636568210252417259%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=WiX72FkUnqkL9WZCbPIe6R4Cl9lFptO3yRXcQv%2BDeow%3D&reserved=0.

lastcoolnameleft commented 6 years ago

This is now implemented to use the value here: https://github.com/Azure/simdem/blob/simdem2/simdem/simdem.ini#L5

SorraTheOrc commented 6 years ago

The variable name is defined as "temp_dir" rather than "SIMDEM_TEMP_DIR". Is there a reason for this? I'd much rather we stay backward compatible because:

lastcoolnameleft commented 6 years ago

"temp_dir" is what it's called inside the simdem.ini file it gets mapped to SIMDEM_TEMP_DIR inside the coe. I can change the name if you want me to, but I figured it would be useful to keep it consistent to *.ini settings.

Here is where it's set to SIMDEM_TEMP_DIR: https://github.com/Azure/simdem/blob/simdem2/simdem/mode/common.py#L138

SorraTheOrc commented 6 years ago

Ahhh.. OK. That's confusing, but it's only a documentation thing.