[MISC] Update data-platform-libs: data_interfaces to 34 and upgrade to 16 #454
Conversation
lucasgameiroborges
requested review from
dragomirp,
marceloneppel and
taurus-forever
tests/unit/test_charm.py
Outdated
| harness.get_relation_data(rel_id, harness.charm.unit.name)["password"] | ||
| == "test-password" | ||
| ) | ||
| assert harness.charm.get_secret("unit", "password") == "test-password" |
There was a problem hiding this comment.
We shouldn't re-write the old test.
We should keep Juju2 compatibilty.
Please add a separate test for secrets, that's only executed when secrets are available
There was a problem hiding this comment.
I kept the old test, running on juju2 only, as it checks the relation databag for the secret. I kept the changed test running on both versions as it is compatible either way. Left some comments for future perspective. LMK what you think
There was a problem hiding this comment.
All right, I see, sorry it was my confusion.
Now I understand what you mean, renaming was a good idea here :-)
All good on this one, thank you :-)
There was a problem hiding this comment.
Actually... My bad sorry, I overlooked this test.
This one is not using secrtes.
IT's only using secrets when secrets are configured but otherwise it's falling back to the databag.
We should have a test that is specifically checking if a Juju secret comes to existence when we create one with charm.set_secret().
I think we had it already somewhere lower down in this file.
We don't need to duplicate that.
I mean: it's good to have a single generic test on set-get secret functionality. It will work both on databag and with Juju Secrets, with no knowledge of the underlying storage.
However we also MUST have specific tests for both databag usage (@only_without_juju_secrets -- should fail otherwise) and vica versa.
I think I have some of those in all PG apps main right now. That concept has to be kept and followed if we want a single pipeline.
tests/unit/test_charm.py
Outdated
| harness.get_relation_data(rel_id, harness.charm.unit.name)["password"] | ||
| == "test-password" | ||
| ) | ||
| assert harness.charm.get_secret("unit", "password") == "test-password" |
There was a problem hiding this comment.
All right, I see, sorry it was my confusion.
Now I understand what you mean, renaming was a good idea here :-)
All good on this one, thank you :-)
|
|
||
| def test_on_get_password(harness): | ||
| # Create a mock event and set passwords in peer relation data. | ||
| harness.set_leader(True) |
There was a problem hiding this comment.
All units should be able to get the password. If now we reduce it to the leader, we're changing the expected behavior instead of the previously expected behavior of the application.
I mean: earlier this test was ensuring that all units get the password (in a way as described by the test).
Setting the unit to leader, from now on this test only ensures (expect) that we can get the password this way for the leader.
Tests generally mustn't change -- except when being aligned to a new behavior (i.e. fix or feature) we are implemneting. However, the changes we did in the charm should have been fully identical to the previous behavior.
There was supposed to be no behavior change in this PR -- thus the tests have to stay intact, too.
Was there a reason for switching to leader? I may be overlooking something, perhaps we did a change that stopped the test from passing as before (and we may want to investigate that).
There was a problem hiding this comment.
Yes, this specific test was failing after the library bump.
My reason to add it was: I realized that this exact same test on the postgreSQL VM repo had set_leader(True) in it (this commit, we can see it being added in the diff). At first I saw no reason for this test to be different on k8s and vm, so I added set_leader(True) here in order to make them coherent with each other, and this change made it pass again.
I thought this was a fair addition to the test, since the same exact test over on VM had it in the first place (not sure why we didn't have it here). If there was a reason for it to not be here, then I might need some more context and suggestions on what to do about it (if not having this).
I think its a good idea to have @marceloneppel and @dragomirp input here, as they might have context that I lack.
There was a problem hiding this comment.
@juditnovak I think the issue is that fetch_my_relation_field is marked as leader only:
https://github.com/canonical/postgresql-k8s-operator/blob/lucas/update-lib/lib/charms/data_platform_libs/v0/data_interfaces.py#L1543
Looked into it more and translation from peer data in get_secret is happening only if there is a difference in keys. I'd guess it works on leader due to some hook firing and translating to secrets.
There was a problem hiding this comment.
I'd propose merging this as is, until we establish the correct behaviour of the translation logic, since we already have it everywhere else.
There was a problem hiding this comment.
Sure @dragomirp if U prefer, go head.
There was a problem hiding this comment.
Approving with suggestion that https://github.com/canonical/postgresql-k8s-operator/pull/454/files/506c0bbe4b47c0d833ad762acf3ec91afa29f0d0#r1586032947 should be understood and followed up.
|
Following our discussion about why |
|
I think that's fine to merge it as is and investigate later. |
…o 16 (canonical#454) * update libs * fix unit tests * add peer check * adapt src code and unit tests * separate tests in 2 versions * review suggestions * add new review suggestions
5 participants
No description provided.