-
Notifications
You must be signed in to change notification settings - Fork 0
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
Initial suggestions for plugin release #2
Comments
@madhushreeray30 😂 Looks like too many rules, I will try to update them. But should I need to update all of them? |
@huacnlee Apologies for the late reply, It seems like a lot of rules however this is the basic structure we follow for all the plugins. It would be great if you could update the plugin as required. Thanks! |
I can't write all table's complete documentation in right now, because there have a lot of details of each table, it related to the business logic on https://open.longportapp.com. And as steampipe-plugin the users just need to know the table's name and the columns, so I think it's enough for now. The other things I think I have done before last commit, if the doc can ignore please check out it again. |
Thanks @huacnlee for this new plugin. Great work 🎉 !!
The basic structure looks good so far. While using the plugin, we did come up with a few suggestions based on our best practices:
config/longport.spc
Could you please update the
longport.spc
file to follow the format of the namecheap plugin.The plugin name in the connection should be
plugin = "longportapp/longport"
.Can you please give a brief description of the config parameter that tells which of them are required and which of them are optional, and then set the parameter with a demo value that looks similar to the actual value?
Also add the details of the environment variables that we can use instead of the config parameters.
Both the descriptions and the config parameters should be commented out and the values should be in the format of the actual values (however incorrect).
For instance:
(The values that I have provided are truly demo values can you replace them with values that look like the original ones although changing parts of them to maintain privacy)
docs/LICENSE
Please add the doc license to the docs folder.
docs/tables/*
Please make sure the table names are singular, for instance,
longport_broker
instead oflongport_brokers
.We do not generally keep the output of the example queries. Is there a specific reason for doing that?
We have slightly changed the pattern of the table docs can you please take this reference and change it to this format. It should include queries in both Postgres and SQLite.
Please update the format of the example queries using https://www.freeformatter.com/sql-formatter.html#before-output (2 indent spaces).
It's better to avoid the use of
select *
instead we can just query the important columns.The table docs look very light, can you please add 2-3 more example queries to each table.
docs/index.md
Could you please update the
index.md
file to follow the format of the namecheap plugin.Please add
engines: ["steampipe", "sqlite", "postgres", "export"]
in the top sectionUpdate the steampipe definition to
[Steampipe](https://steampipe.io) is an open-source zero-ETL engine to instantly query cloud APIs using SQL.
Change the
Get Started
toQuick Start
and add the document section before it.Download and install the latest Twitter plugin:
to use Longport instead of Twitter.Configure your account details in ~/.steampipe/config/longport.spc:
in the configuration section.longport.spc
file.Get Involved
section and add the details to set these parameters via the environment variables.README.md
Could you please update the file to follow the format of the namecheap plugin.
The basic sections are missing so it would be better if you could follow the format from the above reference and then we can further see if any changes are required.
longport/columns.go
We generally keep the columns in the respective table codes and if there are common columns used across we add it to a
common_collumns.go
file. Could you please shift the columns to the respective tables and if any common column is used keep it here after changing the file name tocommon_collumns.go
.Each column description should end with a
.
Do we need to add the transform function in all the columns? The default conversation is from camel case to snake case so I think we do not need the transform when we are keeping the column names same just changing from camel case to snake case. Eg >
TradeStatus
would be changed by GO totrade_status
without any transform function.longport/connection_config.go
Please update the file name from
config.go
toconnection_config.go
Update the
cty
tohcl
and remove theConfigSchema
(as per our latest changes across all plugins)longport/longport_*.go
Please give the table description in the form of a complete sentence. For some tables, it is a bit messy.
Please update the file name to use singular naming for example
longport_broker.go
Please update the function name to the format
tableLongportBroker
. In some tables, we have followed thelongport_history_executions
format which should also be updated totableLongportHistoryExecutions
. Apply to all tables.Whenever we get an error we should print the type of error in the logger for example
plugin.Logger(ctx).Error("longport_broker.listBrokers", "connection_error", err)
when the error is during client creation andplugin.Logger(ctx).Error("longport_broker.listBrokers", "api_error", err)
when the error is during the API call. Please apply this to all tables. (reference)longport/plugin.go
The table names should be sorted in ascending order. (Make sure to use the updated function names).
We no longer need the
ConfigSchema
so we can remove it..github
The folder does not have all the required files can you please update it taking reference from here.
DEVELOPMENT.md
andversion.json
We do not require these files as the information in README and index docs are sufficient.
go.mod
Please update the steampipe-plugin-sdk to
v5.8.0
CHANGELOG.md
Please draft an initial CHANGELOG that will include the following
LICENSE
Please update the file similar to this.
Makefile
Please update the make command to
A couple of follow-up questions:
#3How are we handling the resource not found errors?
Do the APIs support pagination? I did not find any reference to it, but I want to make sure that we have checked that box before releasing it.
While working with the APIs did you encounter rate limit errors?
Please refer https://steampipe.io/docs/develop/plugin-release-checklist#data-ingestion for more information
Please let us know if you have questions, happy to help 👍.
The text was updated successfully, but these errors were encountered: