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

Implement PiHole API token #580

Merged
merged 4 commits into from
Feb 19, 2023
Merged

Implement PiHole API token #580

merged 4 commits into from
Feb 19, 2023

Conversation

fmunteanu
Copy link
Contributor

Description

When PiHole web interface is password protected, a query string containing an API token is required for validation.

Fixes #579

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've read & comply with the contributing guidelines
  • I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • I have made corresponding changes to the documentation (README.md).
  • I've checked my modifications for any breaking changes, especially in the config.yml file

@netlify
Copy link

netlify bot commented Dec 27, 2022

Deploy Preview for homer-demo-content ready!

Name Link
🔨 Latest commit 0a884d6
🔍 Latest deploy log https://app.netlify.com/sites/homer-demo-content/deploys/63f284157a4df70008b32312
😎 Deploy Preview https://deploy-preview-580--homer-demo-content.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.

@@ -49,7 +52,8 @@ export default {
},
methods: {
fetchStatus: async function () {
const result = await this.fetch("/api.php").catch((e) => console.log(e));
const result = await this.fetch("/api.php${this.apiQuery}")
Copy link
Contributor Author

@fmunteanu fmunteanu Dec 27, 2022

Choose a reason for hiding this comment

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

Might require backticks for ${this.apiQuery}, since is a function.

Copy link

Choose a reason for hiding this comment

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

I believe it should be:

const result = await this.fetch(`/api.php${this.apiQuery()}`)

@@ -37,6 +37,9 @@ export default {
ads_percentage_today: 0,
}),
computed: {
apiQuery() {
return this.item.apikey ? "?summaryRaw&auth=${this.item.apikey}" : "";
Copy link

Choose a reason for hiding this comment

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

I believe this needs to be:

return this.item.apikey ? `?summaryRaw&auth=${this.item.apikey}` : "";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tibbe I was looking at other files and @bastienwirtz is usually adding backticks to the function call, something like:

const result = await this.fetch(`/api.php${this.apiQuery()}`)

Copy link

Choose a reason for hiding this comment

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

I believe you need the backticks every time you do variable interpolation (i.e. every time you mention ${..}). which happens on this line. Perhaps it's possible to delay the interpolation but that seems error prone (e.g. the interpolated expression might not be in scope at the later point).

Copy link
Contributor Author

@fmunteanu fmunteanu Dec 31, 2022

Choose a reason for hiding this comment

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

@tibbe thank you for the info, not experienced with the language, Python head here. :) Basically, I should add the backticks in both locations, right? If yes, I'll push the changes now, let me know please.

Copy link

Choose a reason for hiding this comment

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

Yes I believe they need to be in both places and contain the entire string (like I did I my examples).

Copy link
Contributor Author

@fmunteanu fmunteanu Jan 1, 2023

Choose a reason for hiding this comment

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

@tibbe, I pushed the change. Feel free to resolve the conversation. :)

Copy link
Owner

@bastienwirtz bastienwirtz left a comment

Choose a reason for hiding this comment

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

Just saw your open issue and this PR. Thanks @fmunteanu !
Looks good, a few little thing and we should be good to go.

src/components/services/PiHole.vue Outdated Show resolved Hide resolved
src/components/services/PiHole.vue Outdated Show resolved Hide resolved
src/components/services/PiHole.vue Outdated Show resolved Hide resolved
@bastienwirtz
Copy link
Owner

Thanks for the adjustment @fmunteanu I will test it asap.

@bastienwirtz bastienwirtz merged commit d362add into bastienwirtz:main Feb 19, 2023
@bastienwirtz
Copy link
Owner

Thanks again @fmunteanu 👍

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.

PiHole stats not available in Homer if WebUI is password protected
3 participants