-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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: Supervisord input plugin #9015
Conversation
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.
🤝 ✅ CLA has been signed. Thank you!
Updating to 1.18
Hi @niasar! Thanks for the PR! Could you resolve the conflicts so we can get this reviewed? |
Hey, thank you for showing interest. I'll try to do it at this or next weekend. |
# Conflicts: # docs/LICENSE_OF_DEPENDENCIES.md # go.mod
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
@sjwang90 Done |
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.
Hi! This showed up in my list of things to look through today. It looks like it has sat for a while, but I still wanted to give it a review. At a high level, I appreciate that you took the time to write this and include not only unit tests, but an integration test.
I have added a few comments and one big question around username/password. Before I dive into the code, I wanted to see your thoughts on that area first and if you were still interested in looking through this.
Thanks!
… disable basic auth. Added details about using plugin with authentication enabled.
📦 Looks like new artifacts were built from this PR. Expand this list to get them here! 🐯Artifact URLs |
when will this PR be merged? @niasar |
@niasar Can you edit the title of the pull request to |
Additionally please rebase on master to resolve the conflict on @srebhan would be a good one for you to quickly look over :) |
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.
Hi, @niasar wanted to check in and see if you were planning any further updates! Thanks! |
Since this already has one review approval and is only waiting on small docs change requests from me, if @niasar doesn't come back I think we should complete it instead of closing due to inactivity. |
Hello and sorry for absence. As I'm now moved to a new place, i had shortage of free time for a few months. I'll be ready to get back to work in the beginning of next week. |
Thanks for feedback, i'll look in your suggestions later, for now i'm going to merge latest master branch and make integration tests using testutil.container. |
Hey @niasar, thanks for your effort! It seems like the license file needs one line removed to make the tests happy. You can run |
And another conflict @niasar. Let me know if you cannot find time and I can resolve this for you... |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Thanks for finishing this up!
Can I please ask you to put a README on your dockerhub page? Something that lays out 1) the port used and 2) where the configuration file exists?
Hello, I've seen some interest(#5126) in adding input plugin for monitoring processes, that running under supervisord plus maybe supervisord itself and I am interested in it as well. Unit and integration tests are included.
Required for all PRs: