-
-
Notifications
You must be signed in to change notification settings - Fork 672
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
Making logging configurable #60
Conversation
It seems something is wrong with CI setup, can you take a look?
|
Hi @antonbabenko / @brandoconnor Can this be reviewed? sorry to bother you but I want to see if it is possible to get this faster since it is such a small thing |
Hi @mohsen0 ! Yes, I can review, but I am not feeling comfortable with everything that is going after (eg, automated tests, release). @brandoconnor usually reviews such PRs pretty fast. |
variables.tf
Outdated
@@ -80,8 +80,14 @@ variable "load_balancer_update_timeout" { | |||
default = "10m" | |||
} | |||
|
|||
variable "log_enable" { | |||
description = "Enable logging" | |||
default = false |
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 would prefer to set the default to true
to keep backward compatiblity for people who are using master
branch of this module.
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.
ok, changed
Hi @brandoconnor , can you review this work? |
@mohsen0 - I've had a busy week but should be able to release this by end of day. I think the change looks fine. I wonder if there are any combinations of |
RE: CI - I don't know how to allow PRs to initiate CI using the secret environment variables provided to Travis. There are some docs on it here: None of that seems to indicate there's a way to allow PRs to use environmnet variables that are secrets but maybe I've misread. That's the reason why CI is failing. |
I'm going to make a change or two once this is merged but pulling this in now. Thanks for the contribution @mohsen0 |
Ugh and the problem was not too far from my comment. If you try to specify @mohsen0 did you test any of that before submitting? I can think of different ways to make this work happen but it's somewhat clunky. I won't be cutting this in a release until the problem is resolved. |
Sorry not back at this yesterday, will respond to your points later today |
It's a shame to see contributors not taking ownership of their changes. I'll be rolling this back now. |
This reverts commit dd8876d.
I reverted back my changes, I did spend time on this, it will be complicated to keep the current default variables the same thing and make this change work. I can make that change if you are happy with that :), |
Thank you. Accepting that PR now. Re: the count on the two |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
No description provided.