-
Notifications
You must be signed in to change notification settings - Fork 44
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
Migrate gantt chart to OpenSearch Dashboards #1
Conversation
@@ -7,8 +7,8 @@ on: | |||
- 'v*' | |||
|
|||
env: | |||
PLUGIN_NAME: opendistroGanttChartKibana | |||
OD_VERSION: 1.13.0.0 | |||
PLUGIN_NAME: ganttChartDashboards |
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 wonder whether this should be ganttchartDashboards
, but also maybe it doesn't matter, @saratvemulapalli ?
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.
Yeah I would prefer to have ganttchartDashboards
.
Looking other plugins, looks like few of them do follow camel case, and few don't. (I looked at AD, Alerting and security).
Understanding the reason behind it, because it is a file name folks prefer to start with lowercase.
No hard rules, but it would be nice to have it consistent.
Consider fixing CI before merging this. |
|
||
- name: Kibana Pluign Bootstrap | ||
- name: Pluign Bootstrap |
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.
Super nit
- name: Pluign Bootstrap | |
- name: Plugin Bootstrap |
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! addressed comments
@@ -7,8 +7,8 @@ on: | |||
- 'v*' | |||
|
|||
env: | |||
PLUGIN_NAME: opendistroGanttChartKibana | |||
OD_VERSION: 1.13.0.0 | |||
PLUGIN_NAME: ganttChartDashboards |
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.
Yeah I would prefer to have ganttchartDashboards
.
Looking other plugins, looks like few of them do follow camel case, and few don't. (I looked at AD, Alerting and security).
Understanding the reason behind it, because it is a file name folks prefer to start with lowercase.
No hard rules, but it would be nice to have it consistent.
MAINTAINERS.md
Outdated
|------------------------|---------------------------------------------------|-------------| | ||
| Anantha Krishna Bhatta | [akbhatta](https://github.com/akbhatta) | Amazon | | ||
| Anirudha (Ani) Jadhav | [anirudha](https://github.com/anirudha) | Amazon | | ||
| Charlotte | [CEHENKLE](https://github.com/CEHENKLE) | Amazon | |
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.
Also I believe this should just be the folks who are making commits to the repo. It's not just all folks who have write access. As an example: probably Nick and Charlotte are just admins for the team but I do not foresee them making commits or reviewing PRs.
The idea of Maintainers.md is to help community to know whom to reachout.
Here is how the conversation started: https://discuss.opendistrocommunity.dev/t/preparing-opensearch-and-opensearch-dashboards-for-release/5567/3
Also comment from Dawn helps it clarify: "Typically, the maintainers file would just contain the maintainers for this particular repo. This helps new contributors understand who is likely to merge their PRs and gives them real people to follow up with if there are any issues getting their contributions merged."
opensearch-project/OpenSearch#523 (comment)
@dblock any thoughts?
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.
100% agreed
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.
got it, fixed
Issue #, if available:
Description of changes:
TODO left: need to update github workflows with OpenSearch Dashboards repo link and new S3 bucket
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.