-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: add autodiscover enabled feature #3895
feat: add autodiscover enabled feature #3895
Conversation
a4b3b83
to
ff1abd7
Compare
0569d74
to
4ac0b68
Compare
@jskrill if you have time, could you test out this build in your setup? I don't know if you know this but you can create a PR in your fork to merge your feature branch into your fork's Thanks again for all your efforts. |
@nitrocode I built ghcr.io/jskrill/atlantis:v0.27.0 if you wanna try it out. I've been testing all this while with Atlantis deployed to my local k8s cluster (using Tilt to automate the build/deploy cycle). I've been using a PR in this repo for manual testing: jskrill/terraform-atlantis-test#9 As far as unit tests go, I'm think the ones I've written cover the feature pretty well. Let me know if you want any additional testing evidence/etc. Thanks. |
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.
just a few quote suggestions
a0ccefc
to
a030700
Compare
a030700
to
dcd5851
Compare
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.
Thank you!
I'll defer to another maintainer to review and merge.
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.
Looks good to me! The buildAllCommandsByCfg
function is getting pretty long and hard to read with all its if/elses, but that's a pre-existing condition. I left a few small comments
Are we good to merge? |
Polite bump on this. Are there any other changes needed? |
@jskrill LGTM Thanks for the contribution |
Thanks for all your work @jskrill ! |
* add flag to allow the user disable the autodiscover * add global config and doc * feat: Implement autodiscover.mode * fix: Minor doc fixes * fix: Small fixes to docs/indent/tests * fix: Line length, quoting, function comments * fix: Add a few more tests and remove newlines * fix: Always camel case never snake --------- Co-authored-by: Marcelo Medeiros <[email protected]> Co-authored-by: nitrocode <[email protected]> Co-authored-by: PePe Amengual <[email protected]>
* add flag to allow the user disable the autodiscover * add global config and doc * feat: Implement autodiscover.mode * fix: Minor doc fixes * fix: Small fixes to docs/indent/tests * fix: Line length, quoting, function comments * fix: Add a few more tests and remove newlines * fix: Always camel case never snake --------- Co-authored-by: Marcelo Medeiros <[email protected]> Co-authored-by: nitrocode <[email protected]> Co-authored-by: PePe Amengual <[email protected]>
what
why
tests
references