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

Drone io widget #1037

Merged
merged 9 commits into from
Jan 22, 2023
Merged

Drone io widget #1037

merged 9 commits into from
Jan 22, 2023

Conversation

m42e
Copy link
Contributor

@m42e m42e commented Jan 7, 2023

m42e ✨ Feature Medium m42e /drone-io-widget → Lissy93/dashy Commits: 9 | Files Changed: 3 | Additions: 276 Unchecked Tasks

Thank you for contributing to Dashy! So that your PR can be handled effectively, please populate the following fields (delete sections that are not applicable)

Category: Feature

Overview
Added Widget for Drone.io build status

New Vars
Configuration for the plugin:

Field Type Required Description
host string Required The histname of the drone.io instance
apiKey string Required The API key (https:///account)
limit integer Optional Limit the amounts of listed builds.

Screenshot (if applicable)
Screenshor

Code Quality Checklist (Please complete)

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors
  • (If a new config option is added) Attribute is outlined in the schema and documented
  • (If a new dependency is added) Package is essential, and has been checked out for security or performance
  • Bumps version, if new feature added

@m42e m42e requested a review from Lissy93 as a code owner January 7, 2023 00:39
@netlify
Copy link

netlify bot commented Jan 7, 2023

Deploy Preview for dashy-dev ready!

Name Link
🔨 Latest commit db8abb2
🔍 Latest deploy log https://app.netlify.com/sites/dashy-dev/deploys/63bb40139283060008d14738
😎 Deploy Preview https://deploy-preview-1037--dashy-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@viezly
Copy link

viezly bot commented Jan 7, 2023

Changes preview:

Legend:

👀 Review pull request on Viezly

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

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

Just one small thing left to do, the widget needs to be registered.
If you add: 'drone-io': 'DroneIo', into WidgetBase.vue

(the first part is the widgets ID, and is used for type: widget-name in the config, and the second part needs to match the filename)

After that, and the few minor changes in the docs, it all looks good to me :)

I don't have access to a Drone instance to hand to test the functionality, but your code looks good, so I trust it works if you've tested it :)

docs/widgets.md Outdated Show resolved Hide resolved
docs/widgets.md Outdated Show resolved Hide resolved
docs/widgets.md Outdated Show resolved Hide resolved
@Lissy93 Lissy93 added the ✨ New Feature [PR] Contains implementation of a new feature label Jan 7, 2023
@Lissy93
Copy link
Owner

Lissy93 commented Jan 7, 2023

There's also 2 security hotspots raised by SonarCloud (here), both relate to piping the response strait into the href attribute of the link. But if we can trust the response from the API, then I believe in this instance those should be fine. That's the reason for the failing check.

Drone CI is the current term used for the Build System (or just Drone,
which is a bit to unspecific for identification)
@m42e
Copy link
Contributor Author

m42e commented Jan 8, 2023

What would be the alternative way to do it?
Is there a way to process urls returned to somehow ensure they are ok?
As drone ci is self hosted and open source would it be considered trustworthy?

The other points I already changed. Still I'm improving the display of the information a bit. Also renamed it from drone.io to Drone CI (Where Drone is the current name but it is too vague imho).

@m42e
Copy link
Contributor Author

m42e commented Jan 22, 2023

@Lissy93 anything missing here?

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

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

Looks good!

Sorry for the delay, this totally slipped my mind 😬

@Lissy93 Lissy93 merged commit a91a283 into Lissy93:master Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ New Feature [PR] Contains implementation of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants