marekjalovec / steampipe-plugin-make

Use SQL to instantly query Make resources. Open source CLI. No DB required.
Apache License 2.0
5 stars 1 forks source link

Initial suggestions for plugin release #1

Closed misraved closed 1 year ago

misraved commented 1 year ago

Hey @marekjalovec, great work on the new plugin 👍.

The basic structure looks good so far. While using the plugin, we did come up with a few suggestions based on our best practices:

  1. The column descriptions of tables appear to be incorrect. For instance, in https://github.com/marekjalovec/steampipe-plugin-make/blob/main/make/table_make_api_token.go#L21-L24 the column names do not match the description. Could you please update them? Certain descriptions are incomplete as well; the datastructure_id of the make_data_store table could use a generic description like Data structure ID..

  2. The client folder can be moved out of the make folder to make sure that we keep the table code separate from the client connection code. Please take a look at https://github.com/turbot/steampipe-plugin-turbot for more information.

  3. Is there a specific reason for defining separate logger functions? I think we could easily use the format that we follow for other tables. You could refer https://github.com/turbot/steampipe-plugin-pagerduty/blob/main/pagerduty/table_pagerduty_incident.go#L196 for more information. This will help us in maintaining consistency across all the plugins.

  4. Could you please sort the tables in https://github.com/marekjalovec/steampipe-plugin-make/blob/main/make/plugin.go#L28-L39. We follow this format across all the plugins.

  5. config/make.spc file contains great content 👍.

  6. The queries in the table docs look great, but they are light in my opinion. It would be great if you could add 2-3 more example queries per table. Additionally, could you also format the queries using the SQL formatter ? Apologies that this requirement is not properly highlighted in the release checklist.

  7. Could you please remove the query outputs from the table docs? As per our current structure, we do not include query outputs in our table docs 👍. Also, I feel we do not need separate headers for highlighting the Key Columns and Caveats. These descriptions can be included in the table description section of the docs as well.

  8. Does the SDK support environment variables? If yes, I think we should add support for them in the plugin code and mention them explicitly in the docs as well.

  9. All the tables are missing logging information when the API client function is invoked. Could you please add that across all the tables as per the format in https://github.com/turbot/steampipe-plugin-pagerduty/blob/main/pagerduty/table_pagerduty_incident.go#L194-L198

  10. Is there a specific need for dev.sh script? Can we remove it?

  11. Could you please update the .gitignore file to include the extensions mentioned in https://github.com/turbot/steampipe-plugin-pagerduty/blob/main/.gitignore

  12. The plugin seems to be missing the .goreleaser.yml file. Could you please add that in as well 👍 .

  13. The go.mod file is using 4.1.9 Steampipe Plugin SDK version. Could we please update it to use 5.0.2 SDK version.

Overall the plugin looks great 👍. Amazing to see how you made sure that pagination and rate limits are handled properly 👍.

Please let us know if you have questions or if you need a discussion on the above feedback. Happy to help 😄 .

misraved commented 1 year ago

@marekjalovec Could you please add in the docs/index.md file? This file is used as the landing page for the plugins on the hub. You could follow the format used in https://hub.steampipe.io/plugins/turbot/algolia for more information.

The README file could also be updated to use the format followed in https://github.com/turbot/steampipe-plugin-algolia 👍.

misraved commented 1 year ago

@marekjalovec Is there any specific query you would like to use in the social graphics of the make plugin?

In the README file, you have used select c.id, c.name from make_connection c, I think we do have a scope of using a simple, yet powerful query for highlighting this plugin. You could also do away with the aliases, for instance, a query for the social graphics could look something like this:

select
  id,
  name,
  email,
  last_login
from
  make_user;

Please let me know your thoughts on this query 👍 .

marekjalovec commented 1 year ago

Thanks, @misraved!

1] Done, good catch! A couple of descriptions indeed needed updates. 2] Done 3] Syntactic sugar, but it was unnecessary. Removed. 4] Done 5] Thanks! 6] Added a couple 7] Done 8] No SDK and standard env vars exist 9] It's there, just a couple of lines lower than you expect it 10] It's useful for my development flow, but it's now removed from the repo 11] Done 12] Done (taken from another plugin) 13] Done (the updates were fairly small) "14"] index.md added "15"] that's a good one, but needs distinct thanks to the way the API returns data (having a built-in de-dupe method based on key column would be nice):

select distinct
   id,
   name,
   email,
   last_login 
from
   make_user;

Also, if you need the Make logo, it can be taken from make.com, or their github account