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

Update README.md #6

Closed
wants to merge 1 commit into from

Conversation

christianboyle
Copy link

I was setting this up with Synology Container Manager + docker-compose.yml seemed like it wasn't working due to some networking issue.

Like a buffoon, I didn't realize that the labels: are required and that's why I was seeing No containers found in the widget display.

I think maybe the project README.md could be made more clear by:

  • Stating the label is required and having the included example use the assumed existence of a Glance container/project, rather than Sonar, since it's a more obvious example
  • Add a commented out line at the bottom of the docker-compose.yml example in the instructions, like # labels: [...], so that users realize there's more required fields
  • Remove the glance.X.group from the included example to simplify things to the minimum needed to work with a single container (this removes the need to update glance.yml with the group, which should avoid issues like #4

I was setting this up with Synology Container Manager + docker-compose.yml seemed like it wasn't working due to some networking issue.

Like a buffoon, I didn't realize that the `labels:` are required and that's why I was seeing `No containers found` in the widget display.

I think maybe the project `README.md` could be made more clear by:

- stating the label is required and having the [included example](https://github.com/DVDAndroid/glance-docker-container-ext?tab=readme-ov-file#configuration) use the assumed existence of a `Glance` container/project, rather than `Sonar`, since it's a more obvious example
- add a commented out line at the bottom of the `docker-compose.yml` example in the [instructions](https://github.com/DVDAndroid/glance-docker-container-ext?tab=readme-ov-file#installation), like `# labels: [...]`, so that users realize there's more required fields
Copy link
Owner

@DVDAndroid DVDAndroid left a comment

Choose a reason for hiding this comment

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

Thanks for contribution; I have some doubts on these changes.

I've tried to write the docs the more generic possible

README.md Show resolved Hide resolved
Comment on lines +57 to 59
## Configuration (required)

Then, for every container you want to monitor, add the following labels to its compose file:
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, configuration is required if you want to make widget appear in the homepage; in fact the sentence below declares that "for every container you want to monitor" you need to add some labels.

I'll pass on this

README.md Show resolved Hide resolved
Comment on lines +64 to +67
glance.0.name: Glance
glance.0.description: A self-hosted dashboard that puts all your feeds in one place
glance.0.url: https://glance.example.com
glance.0.icon: https://avatars.githubusercontent.com/u/159397742?s=48&v=4
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's not the right example to add.
Nobody will have a widget pointing to glance itself; also this may result confusing for users; they may ask theirself: should I put this on glance container?

I put Sonarr as example to say "your container XYZ". A more generic widget should be provided as example.
What a good example could be? I declare all available properties below, so an example like this

glance.X.name: <name of the container>

isn't quite right

Copy link
Author

Choose a reason for hiding this comment

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

Yea, I can see how this might be more confusing. Thanks for explaining your thinking behind the Sonarr example.

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.

2 participants