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.

Sign up for GitHub

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

Jump to bottom

Fix non-existing errors argument in GrafanaDashboardEvent #74

Merged
merged 2 commits into from
May 11, 2022
Merged

Fix non-existing errors argument in GrafanaDashboardEvent #74

merged 2 commits into from
May 11, 2022

Conversation

rbarry82
Copy link
Contributor

@rbarry82 rbarry82 commented Mar 4, 2022

No description provided.

@rbarry82 rbarry82 requested review from dstathis, davigar15, sed-i and a team March 4, 2022 21:53
@mmanciop
Copy link
Contributor

mmanciop commented Mar 5, 2022

@davigar15 I do not understand the rationale of this PR. Care to comment why this change is needed?

sed-i
sed-i previously approved these changes Mar 8, 2022
View reviewed changes
@rbarry82
Copy link
Contributor Author

rbarry82 commented Mar 8, 2022

@davigar15 I do not understand the rationale of this PR. Care to comment why this change is needed?

Sorted on MM -- it's docs alignment.

@rbarry82
Copy link
Contributor Author

@davigar15 edits from maintainers aren't allowed here. Can you run black --line-length=99 lib/ and re-push?

@rbarry82
Copy link
Contributor Author

@davigar15 ping?

@simskij
Copy link
Member

simskij commented Mar 28, 2022

Pinged @davigar15 on MM.

@davigar15
Copy link
Contributor

Sorry for the late reply. In this line (https://github.com/canonical/grafana-k8s-operator/blob/main/lib/charms/grafana_k8s/v0/grafana_dashboard.py#L900) a kwargs errors is passed to the GrafanaDashboardEvent.

However, the constructor of GrafanaDashboardEvent does not include the the errors kwarg: https://github.com/canonical/grafana-k8s-operator/blob/main/lib/charms/grafana_k8s/v0/grafana_dashboard.py#L658

@simskij
Copy link
Member

simskij commented Mar 28, 2022

Libcheck will always fail on PRs from forks, as they don't have access to the secrets. We should probably make it conditional on the remote branch being local to the repo.

@simskij simskij mentioned this pull request Mar 28, 2022
Closed
@simskij simskij merged commit ea5ec85 into canonical:main May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sed-i sed-i sed-i left review comments

@dstathis dstathis Awaiting requested review from dstathis

@davigar15 davigar15 Awaiting requested review from davigar15

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rbarry82 @mmanciop @simskij @davigar15 @sed-i