-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor!: change envVars to env in func.yaml #316
Conversation
@@ -22,7 +22,7 @@ type config struct { | |||
Trigger string `yaml:"trigger"` | |||
Builder string `yaml:"builder"` | |||
BuilderMap map[string]string `yaml:"builderMap"` | |||
EnvVars map[string]string `yaml:"envVars"` | |||
Env map[string]string `yaml:"env"` |
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.
@lance won't this cause issues for users using newer versions with older conf file?
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.
But not many people use function
so it is not that much of a problem. We just need to let know people that it's been changed.
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 - you are right that any existing users would need to change their config file. But this is all so new, and it's still unsupported, so I think perhaps adding to release notes for TP-2 would be OK instead of trying for backwards compatibility. But I will follow up on this to be sure it's OK.
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.
LGTM!
Yes, this is technically a backwards incompatible change, but let's get this thing squeaky-clean now, before 1.0, while we still can. (I also doubt there are others using env vars other than we team members at this point?),
7ad2b60
to
43eedb0
Compare
I should have noted that this change was prompted by the discussion in #312 (comment). |
@@ -22,7 +22,7 @@ type config struct { | |||
Trigger string `yaml:"trigger"` | |||
Builder string `yaml:"builder"` | |||
BuilderMap map[string]string `yaml:"builderMap"` | |||
EnvVars map[string]string `yaml:"envVars"` | |||
Env map[string]string `yaml:"env"` |
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.
LGTM!
Yes, this is technically a backwards incompatible change, but let's get this thing squeaky-clean now, before 1.0, while we still can. (I also doubt there are others using env vars other than we team members at this point?),
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.
LGTM
Nit: imho this shouldn't be a |
Good point! |
There are some places where I've changed variable and function names where it wasn't strictly necessary. If you don't mind the bit of churn that results, changing these makes `rg -i envvars` a nice way to check for anything that could be overlooked. Signed-off-by: Lance Ball <lball@redhat.com>
43eedb0
to
69f9710
Compare
@zroubalik how about |
There are some places where I've changed variable and function names
where it wasn't strictly necessary. If you don't mind the bit of churn
that results, changing these makes
rg -i envvars
a nice way to checkfor anything that could be overlooked.
Signed-off-by: Lance Ball lball@redhat.com