-
Notifications
You must be signed in to change notification settings - Fork 345
Add support of ENV vars in the config-provider #529
Conversation
…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
if s[i] != '$' { | ||
break | ||
} | ||
varIndex++ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Closing due to inactivity and the fact this is something I'd rather not implement in Prototool |
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:
Also, was extended
generate.go_options.import_path
,lint.file_header.path
,lint.file_header.content
andlint.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.