More changes regarding hasura config

[852Kerfunkle]

No description provided.

[852Kerfunkle]

Regarding this code:

// ConnectionInfo -
type ConnectionInfo struct {
	UsePreparedStatements bool        `json:"use_prepared_statements"`
	IsolationLevel        string      `json:"isolation_level"`
	DatabaseUrl           DatabaseUrl `json:"database_url,omitempty"`
}

// DatabaseUrl -
type DatabaseUrl string

// DatabaseUrlFromEnv -
type DatabaseUrlFromEnv struct {
	FromEnv string `json:"from_env"`
}

// UnmarshalJSON -
func (d *DatabaseUrl) UnmarshalJSON(data []byte) error {
	var s string
	if err := json.Unmarshal(data, &s); err == nil {
		*d = DatabaseUrl(s)
		return nil
	}

	var fromEnv DatabaseUrlFromEnv
	if err := json.Unmarshal(data, &fromEnv); err != nil {
		return err
	}

	*d = DatabaseUrl(fromEnv.FromEnv)
	return nil
}

I think that's broken? From env is not a connection string, it's just an environment variable name? Maybe I'm wrong, but when you serialise that, you'll get

"connection_info": {
    "database_url" : "ENV_VAR_NAME"
}

which will break your hasura config.

I think I'll just revert that to interface{}.

[852Kerfunkle]

I also removed the omitempty on DatabaseUrl, I don't think it can be empty.

[aopoltorzhicky]

I don't think that removing anything is a good idea. I has already fixed database_url in previous commit

[852Kerfunkle]

        fromEnvValue := os.Getenv(fromEnv.FromEnv)
	if fromEnvValue == "" {
		return errors.Errorf("you have to set '%s' environment variable due to hasura connection info", fromEnv.FromEnv)
	}

Heya, I'm not sure that's good way to handle this. Imagine this case:

• Another services connection string is specified in an env var.
• metadata indexer re-writes it to a plain string.
• Env var is updated and the other service and hasura are restarted.
• Hasura now points to the wrong database (assuming the other service does not add the source again).

The point of my PR is to not touch, modify or overwrite other services hasura config, as far as that's possible.

Imo it should either be a union of string, FromEnv and ConnectionParams or simply interface{}, since currently to connection info is not used or modified (except when writing it as a string in AddSource).

[aopoltorzhicky]

Heya, I'm not sure that's good way to handle this. Imagine this case:
Another services connection string is specified in an env var.
metadata indexer re-writes it to a plain string.
Env var is updated and the other service and hasura are restarted.
Hasura now points to the wrong database (assuming the other service does not add the source again).

I'm sorry to be late with the reply. I think that's not problem because different services has to use different environment variables. You should manipulate your connection string by env variables according to hasura docs. But in my opinion we should implement marshaling of from_env DatabaseUrl variant for that case. I'll take care of it in nearest future.

[852Kerfunkle]

I think that's not problem because different services has to use different environment variables. You should manipulate your connection string by env variables according to hasura docs.

Yes, but your change breaks this.

But in my opinion we should implement marshaling of from_env DatabaseUrl variant for that case. I'll take care of it in nearest future.

Ok, works for me.

[852Kerfunkle]

Regardless of your FromEnv change being somewhat broken, I'd appreciate if you merged this and the accompanying metadata PR, simply so I don't need to keep them up to date with other changes to master.

Thanks.

[m-kus]

LGTM