Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Add traceroute-map-panel plugin #675

Merged
merged 10 commits into from
Aug 20, 2020
Merged

Add traceroute-map-panel plugin #675

merged 10 commits into from
Aug 20, 2020

Conversation

Gowee
Copy link
Contributor

@Gowee Gowee commented Jul 11, 2020

Hello.

This panel plugin helps visualize traceroute hops on a map as is shown in the README.

I build this project based upon https://github.com/grafana/simple-react-panel and have checked the relevant guidelines. But I am not sure if everything will go well because the new and legacy documents seems a little messy to me. Please let me know if there is something wrong.

Thanks.

@ganadara135
Copy link

Place your repo.json info on the top of repo.json. It is the way to pass conflicting repo.json.
스크린샷 2020-08-04 10 16 27

@Gowee
Copy link
Contributor Author

Gowee commented Aug 4, 2020

Thanks for reminding me. I have manually resolved those conflicts by rebasing.

@marcusolsson marcusolsson added submission/new Submission is a new plugin type/panel Categorizes the submission as a panel labels Aug 10, 2020
@marcusolsson
Copy link
Contributor

Hi @Gowee, and thank you for considering contributing your plugin!

Before we can start review your plugin, we'll need you to fix the issues reported by the plugin validator. You can enter the URL to this PR, or to a specific commit on GitHub.

@marcusolsson marcusolsson added the milestone/needs-changes Submission needs changes before it can be approved label Aug 11, 2020
@Gowee
Copy link
Contributor Author

Gowee commented Aug 18, 2020

Hi, @marcusolsson .

The validator warns that my panel is Missing dist directory, which, in turn, results in the Invalid ID format and Invalid plugin.json error.
However, my project is following the skeleton of the grafana/simple-react-panel where the .gitginore just ignores the dist/ directory. I was expecting that the plugin would be built automatically somehow when being shipped as a plugin in this repository so that dist/ is unnecessary to be tracked by git. Now I realize the workflow differs from what I have expected.
Then, I am not sure what is the best practice to handle this dist/,

  • Just remove it from .gitignore and add it to git?
  • Or set up a separate release branch to track the dist/ to avoid mess up the version control?

What do you suggest?

Thanks.

@marcusolsson
Copy link
Contributor

As you've noticed, the dist directory needs to be present in the commit. We still have some ways to go when it comes to publishing plugins.

You can find a recommended approach in the tutorial step on how to Publish your plugin.

TL;DR

  • Create a release branch
  • Run yarn build
  • Run git add -f dist to force-add the dist directory

The validator tries to validate the files in the dist directory, but falls back to the src (or root) directory. So the other issues shouldn't be related to the missing dist directory.

@Gowee
Copy link
Contributor Author

Gowee commented Aug 18, 2020

Hmmm. Following the tutorial, I have fixed all the issues reported by the validtor except Invalid ID format & id: Does not match pattern, because I am not sure what is wrong. My panel id is gowee-traceroute-map-panel, where gowee is my registered account on grafana.com.

@marcusolsson
Copy link
Contributor

The plugin ID needs to be 2 or 3 text segments separated by hyphens (yours has 4). A valid ID in your case would be one of these two:

  • gowee-panel
  • gowee-traceroutemap-panel

@Gowee
Copy link
Contributor Author

Gowee commented Aug 18, 2020

Got it. That might be because I saw there are a few plugins that have 4 segments in their IDs in repo.json (for history reason I guess?).

Now changed to gowee-traceroutemap-panel.

@marcusolsson
Copy link
Contributor

Yes, unfortunately a few (including one of my own 😅) plugins have slipped through the review process in the past. Hopefully, the review process will be more consistent and fair as we start automating more of it. Sorry for the confusion!

@marcusolsson
Copy link
Contributor

Just tested the plugin, and it looks like it's working as intended!

Just an FYI: As long as the data frame has the correct schema, you can use any data source. You might want to document the expected data frame in your README. For example, I tested with the Static data source.

Screen Shot 2020-08-19 at 10 34 41

@marcusolsson
Copy link
Contributor

I'd like to congratulate you on finding a gap in the plugin validator! 🎉 😄

I tried publishing your plugin, but wasn't able to. In plugin.json, the screenshot refers to a path in the root directory. When the plugin is built, all required plugin assets needs to be included in the dist directory.

Can I ask that you move the screenshot to the src/img directory (which will be included in the production build)? I'll make sure to add a check in the validator for the future.

@Gowee
Copy link
Contributor Author

Gowee commented Aug 19, 2020

It is a little disappointing that the congratulation is not about my plugin would finally work. 🤣

Certainly, I have corrected the screenshot path. Also, I have updated the README to include the expected data schema. Thanks again for your time and detailed advice.

@Gowee
Copy link
Contributor Author

Gowee commented Aug 19, 2020

It seems that I accidentally include some commits / file changes from #705. It is unexpected and I have no idea how to resolve that. My previous git pull (rebase) trick also does not help. Please let me know if other actions are needed before merging. I think raising a new PR might be a feasible solution?

(Update: Problem solved by some dirty tricks. Now it looks good, at least. )

@marcusolsson
Copy link
Contributor

I promise we'll get there :)

I can see that you've moved the screenshot to src/img, which is great! Unfortunately however, it doesn't seem to be included in dist/img. Did you run yarn build after you moved it? Did you check in the updated dist directory?

It's a really cool plugin, can't wait to publish it!

@Gowee
Copy link
Contributor Author

Gowee commented Aug 20, 2020

I did rebuild and git add -u after merging changes from the master to the release branch. But I forget to git add -f dist/ again, resulting in the screenshot.png is not tracked. Hope there is no omission this time. :-)

@marcusolsson
Copy link
Contributor

Hm. I'm still getting Not Found for the screenshot when I try to publish the plugin. I think it might be because the ./ prefix (which seems silly if it is). I'll ask around to see if this is supported or not. I think it ought to be, but if you don't mind, could you change ./img/screenshot.png to img/screenshot.png?

Thank you for your patience! 🙏

@Gowee
Copy link
Contributor Author

Gowee commented Aug 20, 2020

No problem at all. Updated. 🙂

Gowee/traceroute-map-panel@a59eb13

@marcusolsson
Copy link
Contributor

That was fast! And it indeed did the trick! I'm happy to finally be able to congratulate you on having published your first plugin! 🎉

You can find it here:
https://grafana.com/grafana/plugins/gowee-traceroutemap-panel

Thanks again for your patience and for your contribution! ❤️

@marcusolsson marcusolsson merged commit ba61c20 into grafana:master Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
milestone/needs-changes Submission needs changes before it can be approved submission/new Submission is a new plugin type/panel Categorizes the submission as a panel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants