-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add support for InfluxDB shell Plugin #332
Conversation
Nice! Don't forget to also write an accompanying blog post on Hashnode with the tags 1Password and BuildWith1Password (not just putting #1Password in the text - but use the Hashnode tags in the CMS. :) The full instructions are here. |
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 good start, @bala-ceg. Thank you for your contribution! I have a couple of minor comments to address below, including building support for config file importer.
I am also reviewing how big InfluxDB OSS and InfluxDB Cloud differ, and whether we should build support for both in the same shell plugin. That'd be similar to redis and mongoDB:
- new(redis-cli): Add support for Redis CLI with password as environment variable and command-line flags #276
- new(mongosh): Support mongosh executable for connecting to a MongoDB database #283
I'll followup once I have more to share, but if you are familiar with InfluxDB OSS and InfluxDB Cloud, and on how the CLI interacts with the two, I'd love to hear from you. That'd help me jumpstart the whole process. Thank you!
NeedsAuth: needsauth.IfAll( | ||
needsauth.NotForHelpOrVersion(), | ||
needsauth.NotWithoutArgs(), | ||
needsauth.NotWhenContainsArgs("config"), |
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.
Let's see authentication for version
and completion
. Commands like backup
and restore
are specific to InfluxDB OSS and InfluxDB Cloud, but I am still figuring out whether we should exclude them entirely or build support for InfluxDB OSS too. I'll followup here.
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 have not done any changes to this comment. My assumption we are supporting only influxdb cloud at this moment via shell plugin
hey @bala-ceg We'd love to partner with you to get this over the finish line! |
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 again @bala-ceg, I have been researching a bit about InfluxDB Cloud and InfluxDB OSS. At this time, let's move forward with supporting InfluxDB Cloud 2.0, which the PR does now, and we'll figure out adding support for InfluxDB OSS in the future.
With this said, could we help you with anything in addressing the pending feedback on this PR?
Thanks, Arun. I will address this change by sometime tomorrow |
Hi @techcraver @arunsathiya, made the review changes. I apologise for the delay |
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.
Functionally tested and code changes look good to me. Approving in the spirit of reaching the finish line for this PR, but we'll need to address the config file importer before we release the plugin:
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.
code lgtm.
but we'll need to address the config file importer before we release the plugin:
I think the config file importer should not be a blocker for releasing this plugin, as it already has a valid importer set up. It is indeed a nice-to-have and it's great that we created a separate issue for it!
Hi @bala-ceg, appreciate your contribution! 🙌 The code is almost ready to be merged. All you need to do is sign your commits. You can follow the instructions below to sign your commits.
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
Complete with #392 |
Overview
Added support for influx cli to store the influx environmental variables
Closes #311
Type of change
How To Test
Additional information