Added ACTIVESTATE_CLI_IGNORE_ENV for ignoring env vars during runtime setup.

[mitchell-as]

DX-3011 Support overriding PYTHONPATH

For example, PYTHONPATH.

[Naatan]

This is deleting environment variables regardless of whether we created them. If inherit is true then you might be deleting the very PYTHONPATH the user was trying to set, along with ours.

I think we need to address this inside the envdef package. And it should prevent defining the env var, rather than delete it after the fact (unless we can do it without inheritance playing a role).

[mitchell-as]

No, there's an integration test that verifies we're leaving the user's set PYTHONPATH alone. This deletion only affects the runtime environment, which has not yet been merged with the current one. Since this runtime environment does not contain PYTHONPATH, when merging with the current one, the user's set PYTHONPATH will remain.

I tried to make this happen in envdef, but there are at least three different places where environment mutations happen, and all would need the check:

• cli/pkg/runtime/internal/envdef/environment.go

  Lines 178 to 222 in d893ec6

  	func (ed *EnvironmentDefinition) Merge(other *EnvironmentDefinition) (*EnvironmentDefinition, error) {
  	res := *ed
  	if other == nil {
  	return &res, nil
  	}
  	newEnv := []EnvironmentVariable{}
  	thisEnvNames := funk.Map(
  	ed.Env,
  	func(x EnvironmentVariable) string { return x.Name },
  	).([]string)
  	newKeys := make([]string, 0, len(other.Env))
  	otherEnvMap := map[string]EnvironmentVariable{}
  	for _, ev := range other.Env {
  	if !funk.ContainsString(thisEnvNames, ev.Name) {
  	newKeys = append(newKeys, ev.Name)
  	}
  	otherEnvMap[ev.Name] = ev
  	}
  	// add new keys to environment
  	for _, k := range newKeys {
  	oev := otherEnvMap[k]
  	newEnv = append(newEnv, oev)
  	}
  	// merge keys
  	for _, ev := range ed.Env {
  	otherEv, ok := otherEnvMap[ev.Name]
  	if !ok {
  	// if key exists only in this variable, use it
  	newEnv = append(newEnv, ev)
  	} else {
  	// otherwise: merge this variable and the other environment variable
  	mev, err := ev.Merge(otherEv)
  	if err != nil {
  	return &res, err
  	}
  	newEnv = append(newEnv, *mev)
  	}
  	}
  	res.Env = newEnv
  	return &res, nil

• cli/pkg/runtime/internal/envdef/environment.go

  Lines 370 to 379 in d893ec6

  	func (ed *EnvironmentDefinition) GetEnv(inherit bool) map[string]string {
  	lookupEnv := map[string]string{}
  	if inherit {
  	lookupEnv = osutils.EnvSliceToMap(os.Environ())
  	}
  	res, err := ed.GetEnvBasedOn(lookupEnv)
  	if err != nil {
  	panic(fmt.Sprintf("Could not inherit OS environment variable: %v", err))
  	}
  	return res

• cli/pkg/runtime/internal/envdef/environment.go

  Lines 337 to 362 in d893ec6

  	func (ed *EnvironmentDefinition) GetEnvBasedOn(envLookup map[string]string) (map[string]string, error) {
  	res := maps.Clone(envLookup)
  	for _, ev := range ed.Env {
  	pev := &ev
  	if pev.Inherit {
  	osValue, hasOsValue := envLookup[pev.Name]
  	if hasOsValue {
  	osEv := ev
  	osEv.Values = []string{osValue}
  	var err error
  	pev, err = osEv.Merge(ev)
  	if err != nil {
  	return nil, err
  	}
  	}
  	} else if _, hasOsValue := envLookup[pev.Name]; hasOsValue {
  	res[pev.Name] = "" // unset
  	}
  	// only add environment variable if at least one value is set (This allows us to remove variables from the environment.)
  	if len(ev.Values) > 0 {
  	res[pev.Name] = pev.ValueString()
  	}
  	}
  	return res, nil

I thought it would be better to capture it at the top-level than all those low-level places where things can slip through the cracks if we're not careful.

[Naatan]

I agree having a central place for it would be ideal, but this still feels too error prone to me. The fact that it passes in your tests worries me more than that it settles my concern, because reading through this code suggests that it should not work.

If we are to place the logic here then there needs to be an understanding of why that test is passing, and the code should clearly communicate either via logic or comments that this is working with an awareness of inheritance.

[Naatan]

I agree having a central place for it would be ideal, but this still feels too error prone to me. The fact that it passes in your tests worries me more than that it settles my concern, because reading through this code suggests that it should not work.

If we are to place the logic here then there needs to be an understanding of why that test is passing, and the code should clearly communicate either via logic or comments that this is working with an awareness of inheritance.

[mitchell-as]

Okay, I had the realization I could try to cut of envdef loading at the source, and that appears to work. Perhaps this will be okay?

[mitchell-as]

Order doesn't matter in envdefs, right?

[Naatan]

I don't believe so, but this doesn't seem to change the order?

Note sliceutils.Filter() would be good here (easier to digest).

[Naatan]

I don't believe so, but this doesn't seem to change the order?

Note sliceutils.Filter() would be good here (easier to digest).

[Naatan]

If the first entry matches it seems like you would have an infinite loop here, because i will forever remain 0.