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

refactor: remove multiple relation check in charm code. #229

Merged

Conversation

chanchiwai-ray
Copy link
Contributor

@chanchiwai-ray chanchiwai-ray commented Apr 26, 2024

The metadata.yaml support limit for interface, this can limits how many relation the interface can be connected to, and we can handle the relation checks in juju level.

Also removed the incorrect usage of envdir in tox.ini this is no longer working in tox >= 4.

Old behavior:

$ juju status
hardware-observer/1*  active    blocked            10.42.75.15            Cannot relate to more than one grafana-agent

New behavior:

$ juju relate g2  hardware-observer
ERROR cannot add relation "g2:cos-agent hardware-observer:cos-agent": establishing a new relation for hardware-observer:cos-agent would exceed its maximum relation limit of 1 (quota limit exceeded)

Closes: #144

Pjack
Pjack previously approved these changes Apr 26, 2024
dashmage
dashmage previously approved these changes Apr 26, 2024
Copy link
Contributor

@dashmage dashmage left a comment

Choose a reason for hiding this comment

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

Although I'm in favor of the changes, I think it would be nice to remove the get_num_cos_agent_relations method as well since there would only be either 1 or 0 cos-agent relations now (ie, the relation is either present/ absent). It might be better to have the method be cos_agent_relation_present and use something like self.cos_agent_relation_present == True instead of handling the number of relations now. But if you feel like it expands the scope of the PR too much, we can handle it on separate PR as well 😄

@chanchiwai-ray
Copy link
Contributor Author

Although I'm in favor of the changes, I think it would be nice to remove the get_num_cos_agent_relations method as well since there would only be either 1 or 0 cos-agent relations now (ie, the relation is either present/ absent). It might be better to have the method be cos_agent_relation_present and use something like self.cos_agent_relation_present == True instead of handling the number of relations now. But if you feel like it expands the scope of the PR too much, we can handle it on separate PR as well 😄

good point, the property can be removed and replace with the function call directly. (save few lines of code)

@chanchiwai-ray
Copy link
Contributor Author

Rebased onto master branch

dashmage
dashmage previously approved these changes Apr 29, 2024
@dashmage
Copy link
Contributor

dashmage commented Apr 29, 2024

Func tests are failing due to this issue: charmed-kubernetes/pytest-operator#131

Edit: I've provided a temporary fix with this PR: #232
Edit2: Fix not needed anymore. The func tests work with pytest-asyncio==0.21.2

Pjack
Pjack previously approved these changes Apr 30, 2024
rgildein
rgildein previously approved these changes Apr 30, 2024
@Pjack Pjack linked an issue May 2, 2024 that may be closed by this pull request
@chanchiwai-ray chanchiwai-ray changed the base branch from master to main May 2, 2024 02:34
@chanchiwai-ray chanchiwai-ray dismissed stale reviews from rgildein, Pjack, and dashmage May 2, 2024 02:34

The base branch was changed.

The `envdir` is not supposed to be used that way, and it's broken
"feature" (technically a bug) in tox >= 4. [1]

[1] tox-dev/tox#2788 (comment)
The `metadata.yaml` support `limit` for interface, this can limits how
many relation the interface can be connected to, and we can handle the
relation checks in juju level.
@chanchiwai-ray
Copy link
Contributor Author

rebased onto main

@chanchiwai-ray chanchiwai-ray merged commit 02ba6a5 into canonical:main May 2, 2024
4 checks passed
@chanchiwai-ray chanchiwai-ray deleted the refactor/multiple-relations branch May 2, 2024 04:05
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.

Update status hook not able to start inactive status service.
5 participants