Skip to content
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 streaming category #671

Closed
wants to merge 2 commits into from
Closed

Add streaming category #671

wants to merge 2 commits into from

Conversation

ChrsMark
Copy link
Member

This PR adds streaming category useful to tag stan package, or NATS Streaming.
stan PR: elastic/integrations#532
stan docs: https://github.com/nats-io/stan.go

@elasticmachine
Copy link

elasticmachine commented Jan 18, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #671 updated

    • Start Time: 2021-01-18T14:26:56.300+0000
  • Duration: 6 min 5 sec

  • Commit: 3489847

Test stats 🧪

Test Results
Failed 0
Passed 169
Skipped 0
Total 169

@mtojek mtojek self-requested a review January 18, 2021 14:23
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change coordinated with product team or just your suggestion? I'm wondering if there is some work needed on the Kibana side.

util/package.go Outdated
@@ -44,6 +44,7 @@ var CategoryTitles = map[string]string{
"os_system": "OS & System",
"productivity": "Productivity",
"security": "Security",
"streaming": "Data Streaming"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing trailing comma

@ChrsMark
Copy link
Member Author

Is this change coordinated with product team or just your suggestion? I'm wondering if there is some work needed on the Kibana side.

It's just a suggestion to enrich the tags to better match the specific integration since this is one is better identified as a streaming platform rather than a message queue.

@mtojek
Copy link
Contributor

mtojek commented Jan 18, 2021

It's just a suggestion to enrich the tags to better match the specific integration since this is one is better identified as a streaming platform rather than a message queue.

@ruflin @skh do you think it's safe to push this change (won't blow up anything in Kibana)?

@mtojek mtojek requested a review from skh January 18, 2021 14:43
@mtojek
Copy link
Contributor

mtojek commented Jan 18, 2021

@ChrsMark once we agree on this change, you'll have to update the package-spec as well: https://github.com/elastic/package-spec/blob/master/versions/1/manifest.spec.yml#L64 (then update elastic-package on the spec, but I can help you with this).

@mtojek mtojek requested a review from ruflin January 18, 2021 15:28
@ruflin
Copy link
Contributor

ruflin commented Jan 18, 2021

These categories are synced with https://www.elastic.co/integrations so I would like to pull in @asawariS to chime in if we can just add a category here.

@mtojek
Copy link
Contributor

mtojek commented Jan 18, 2021

These categories are synced with https://www.elastic.co/integrations so I would like to pull in @asawariS to chime in if we can just add a category here.

Fun fact: these tiles hyperlink to Beats documentation (a bit irrelevant).

@asawariS
Copy link

@ruflin - Thanks for flagging. I defer to @tbragin @mostlyjason and other PMs on the tags here. Even though the tags on elastic.co/integrations are not directly currently pulled from the EPR, we eventually want them to be. I agree that there needs to be some process / discussion before new tags are added -- I think PM/Eng is best suited to define that process. we will just consume the final decision on the web page.

Fun fact: these tiles hyperlink to Beats documentation (a bit irrelevant).

@mtojek good catch. When the page was created, most integrations (via agent) didn't exist, so we pointed to Beats modules in most places, etc. There was also the case of do we point people to beta features (not supported in prod) or the GA module version.

Can point me to the appropriate link in the docs for Nats integration. I will discuss with PMs and take point on updating the links for this and other integrations that are now available via Fleet/Agent, if that's the route we go with.

@mtojek
Copy link
Contributor

mtojek commented Jan 19, 2021

Can point me to the appropriate link in the docs for Nats integration. I will discuss with PMs and take point on updating the links for this and other integrations that are now available via Fleet/Agent, if that's the route we go with.

The only (so far best) place you can find documentation for an integration without booting up Kibana is Github:

https://github.com/elastic/integrations/tree/master/packages/nats

@mostlyjason
Copy link

We have a policy to coordinate changes to these categories through ECS. @jamiehynds already has an open PR for updates to these categories in ECS so I recommending syncing with him on this request elastic/ecs#958. I'd recommend moving this request to the ECS repo to coordinate it.

@ruflin
Copy link
Contributor

ruflin commented Jan 20, 2021

@mostlyjason These are NOT the same categories. What is in the package-spec is what is also on the elastic.co/integrations page:

var CategoryTitles = map[string]string{
There have been quite a few historical discussions on this, check also the conversations in the PR you linked.

The question here is: Should we add it to our categories and if yes, it means we must also add it to /integrations. package-spec is currently the source of truth for categories and I think it should stay the source of truth.

@tbragin
Copy link

tbragin commented Jan 25, 2021

What in addition to "NATS Streaming" be part of the "streaming" category? I'd prefer to be quite conservative in adding new categories, we already have quite a few: https://www.elastic.co/integrations We previously said we need at least 5 cards to make a new category.

@ChrsMark
Copy link
Member Author

ChrsMark commented Jan 25, 2021

Ok it seems that we only have this one for now in this category and I can only think of Redis Streaming if we decide to add it in the feature. So leaving it out sounds good to me @tbragin, thanks! I will update the package's description properly to indicate the streaming functionality as well as tag it with k8s category.

@ruflin @mtojek feel free to close this issue if you agree on this

@mtojek
Copy link
Contributor

mtojek commented Jan 26, 2021

Thank you Christos for checking this. I will resolving the issue.

@mtojek mtojek closed this Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants