Skip to content
This repository has been archived by the owner on Mar 4, 2022. It is now read-only.

Add support of ENV vars in the config-provider #529

Closed

Conversation

demdxx
Copy link

@demdxx demdxx commented Dec 11, 2019

To be able to use more flexible file paths was added support of ENV vars in the config-provider.

The main idea is the possibility to use the include paths from the system environment.

For example:

protoc:
  version: 3.8.0

  includes:
    - ".."
    - ${GOPATH}/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.12.1/third_party/googleapis
    - ${GOPATH}/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.12.1

lint:
  file_header:
    content: |
      // ${USER} - Document generation
    is_commented: true

Also, was extended generate.go_options.import_path, lint.file_header.path, lint.file_header.content and lint.ignores.files with the same reason.

NOTE: Originally it was implemented only for several fields of config but can be extended for the whole config file.

…ars in the config-provider.

The main idea is the possibility to use the include paths from the system environment.
For example:

```
protoc:
  version: 3.8.0

  includes:
    - ".."
    - ${GOPATH}/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.12.1/third_party/googleapis
    - ${GOPATH}/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.12.1

lint:
  file_header:
    content: |
      // ${USER} - Document generation
    is_commented: true
```

Also, was extended `generate.go_options.import_path`, `lint.file_header.path`, `lint.file_header.content` and `lint.ignores.files` with the same reason
@claassistantio
Copy link

claassistantio commented Dec 11, 2019

CLA assistant check
All committers have signed the CLA.

@demdxx demdxx changed the title To be able to use more flexible file paths was added support of ENV v… Add support of ENV vars in the config-provider Dec 11, 2019
if s[i] != '$' {
break
}
varIndex++
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty strange way of doing the replacement. Can we make this better? Also, I'm not sure I agree with the replacement rules:

Why does $${PWD} become ${PWD} but $$${PWD} gets the replacement string with an extra $ on the end? That doesn't make a lot of sense.

Copy link
Author

@demdxx demdxx Dec 31, 2019

Choose a reason for hiding this comment

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

Yes, it's not the best way of "char-escaping". Was chosen SQL escape approach like '''' => '' as for configs used YAML and JSON which already supports backslash escaping \n\t\\ what means that to escape the same way need to write \\$ which is also not really pretty, but it's OK to me to use the \\$ if you don't mind.
Or if you have other suggestions please share it to me

Copy link
Contributor

Choose a reason for hiding this comment

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

I really feel like this opens up a bit of complexity with resolving paths and variables, etc. that I'd rather not introduce to Prototool at this point. I apologize.

I would recommend maybe forking the repo and using that fork with your env var changes there.

Copy link
Author

Choose a reason for hiding this comment

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

To use it from my repository a bit harder the original. I think that using the global environment in paths is easier and convenient in many cases, especially it does not affect any other internal logic.

As you can see from the example it makes my life much easier. So if you will decide the way of using global environment variables I will glad to implement it for you and for my self.

PROTOTOOL_VERSION := efdb8d5776a679a0032cfb2c81e17270aa629941
PROTOTOOL := $(TMP_VERSIONS)/prototool/$(PROTOTOOL_VERSION)
$(PROTOTOOL):
	# $(eval PROTOTOOL_TMP := $(shell mktemp -d))
	# cd $(PROTOTOOL_TMP); go get github.com/uber/prototool@$(PROTOTOOL_VERSION)
	# @rm -rf $(PROTOTOOL_TMP)
	@rm -rf $(TMP)/src/prototool
	@mkdir -p $(TMP)/src/prototool
	cd $(TMP)/src/prototool; git clone https://github.com/demdxx/prototool .; \
		git checkout $(PROTOTOOL_VERSION); go build -o ./prototool cmd/prototool/main.go
	rm -f $(TMP_BIN)/prototool
	mv $(TMP)/src/prototool/prototool $(TMP_BIN)/prototool
	@rm -rf $(dir $(PROTOTOOL))
	@mkdir -p $(dir $(PROTOTOOL))
	@touch $(PROTOTOOL)
	@rm -rf $(TMP)/src/prototool

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay, but again, I don't want to put anything regarding env vars in Prototool right now. Why is it harder for you to fork and implement the changes there?

@smaye81 smaye81 closed this Mar 10, 2020
@smaye81
Copy link
Contributor

smaye81 commented Mar 10, 2020

Closing due to inactivity and the fact this is something I'd rather not implement in Prototool

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants