Landscape Charm

Merge lp://qastaging/~lihuiguo/landscape-charm/registration-relation into lp://qastaging/~landscape/landscape-charm/trunk

registration-relation
Merge into trunk

Proposed by Linda Guo on 2021-10-04

Status:

Merged

Approved by:

Simon Poirier on 2021-10-29

Approved revision:

414

Merged at revision:

406

Proposed branch:

lp://qastaging/~lihuiguo/landscape-charm/registration-relation

Merge into:

lp://qastaging/~landscape/landscape-charm/trunk

Diff against target:

189 lines (+121/-1)

8 files modified

config.yaml (+4/-0)
hooks/application-dashboard-relation-changed (+8/-0)
hooks/application-dashboard-relation-joined (+8/-0)
hooks/config-changed (+0/-1)
lib/relations/application_dashboard.py (+62/-0)
lib/relations/tests/test_application_dashboard.py (+35/-0)
lib/services.py (+2/-0)
metadata.yaml (+2/-0)

To merge this branch:

bzr merge lp://qastaging/~lihuiguo/landscape-charm/registration-relation

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
🤖 Landscape Builder	test results		Approve on 2021-10-29
Simon Poirier (community)		2021-10-07	Approve on 2021-10-29
BootStack Reviewers		2021-10-04	Pending
Review via email: mp+409546@code.qastaging.launchpad.net

Commit message

Add application-dashboard relation

  By adding application-dashboard relation in landscape charm,
  we can register landscape in Homer dashboard after
  creating relation between landscape charm and Homer charm

Revision history for this message

Kevin Nasto (silverdrake11) on 2021-10-07:

Revision history for this message

Linda Guo (lihuiguo) wrote on 2021-10-08:

Hi Kevin,

Thanks for the review comments. I have updated the code based on the comments

Revision history for this message

Simon Poirier (simpoir) wrote on 2021-10-08:

I'm a bit puzzled about where this patch comes from. I can't find any other charm with application-dashboard relations, nor any other charm providing it, nor any homer charm.

Could you shed some light over where this came from and how it can be verified?

review: Needs Information

Revision history for this message

James Troup (elmo) wrote on 2021-10-08:

Simon Poirier <email address hidden> writes:

> I'm a bit puzzled about where this patch comes from. I can't find
> any other charm with application-dashboard relations, nor any other
> charm providing it, nor any homer charm.

grafana, graylog, nagios have already been updated to support the
application-dashboard relation. Patches are being reviewed for
OpenStack horizon.

The homer charm is available here:

https://launchpad.net/charm-homer-dashboard

You can also find a document with more details on why we're doing this
here:

https://docs.google.com/document/d/1dWk6W1ykNZobFD7F3Jbwmu1_iNQhf4gsyLmrL-Otjfc/edit#

--
James

Revision history for this message

Simon Poirier (simpoir) wrote on 2021-10-14:

Thanks for the clarification.
I still have a few criticisms, and the homer was utterly broken when I tried it,
but the relation data was present as expected from this MP.

review: Needs Fixing

Revision history for this message

Linda Guo (lihuiguo) wrote on 2021-10-25:

Thanks Simon, I just updated the code according to your comments. Can you please file a bug against homer-dashboard charm if homer is broken? Thanks

Revision history for this message

Simon Poirier (simpoir) wrote on 2021-10-26:

Never mind about the dashboard. I was missing the homer resource in my test bundle.
Looking back at it, I think the proposed code would be more consistent with the existing code if we were using a data provider like for other relations code.

Do you think the following refactor to your branch would make sense? https://pastebin.canonical.com/p/vffVXMCb3y/

Revision history for this message

Linda Guo (lihuiguo) wrote on 2021-10-29:

That looks great! I was thinking using data provider for homer but not familiar with service charm.

I merged your proposal into my branch, deployed it on my local box, it works fine. Can you please review it again? Thanks very much.

> Never mind about the dashboard. I was missing the homer resource in my test
> bundle.
> Looking back at it, I think the proposed code would be more consistent with
> the existing code if we were using a data provider like for other relations
> code.
>
> Do you think the following refactor to your branch would make sense?
> https://pastebin.canonical.com/p/vffVXMCb3y/