Skip to content

Conversation

BalabaDmitri
Copy link
Contributor

@BalabaDmitri BalabaDmitri commented Mar 12, 2024

Issue #444
Simultaneous creation of legacy and modern endpoints relations

Solution
Charm goes into a "blocked" state, when creating a simultaneous relation legacy and modern endpoints.

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.83%. Comparing base (faa360a) to head (50bf245).
Report is 9 commits behind head on main.

❗ Current head 50bf245 differs from pull request most recent head 5474507. Consider uploading reports for the commit 5474507 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   80.96%   80.83%   -0.13%     
==========================================
  Files          10       10              
  Lines        2259     2244      -15     
  Branches      363      362       -1     
==========================================
- Hits         1829     1814      -15     
- Misses        354      356       +2     
+ Partials       76       74       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@taurus-forever
Copy link
Contributor

Hi @BalabaDmintri and thank you for contribution here!
Please sign contribution agreement to merge the PR: https://github.com/canonical/postgresql-operator/actions/runs/8256188989/job/22618992227?pr=396

Tl;DR: Are we testing the right things here? :-D

Long: I am slightly confused. I thought our current position is to disallow simultaneous legacy and modern interfaces: Please choose one endpoint to use. No need to relate all of them simultaneously!.
IMHO, charm should get into 'blocked' state forcing users to remove one relation and choose legacy or modern only.
@marceloneppel is it still valid position? I cannot find such blockers in charm.py code :-(

@marceloneppel
Copy link
Member

Hi @BalabaDmintri and thank you for contribution here! Please sign contribution agreement to merge the PR: https://github.com/canonical/postgresql-operator/actions/runs/8256188989/job/22618992227?pr=396

Tl;DR: Are we testing the right things here? :-D

Long: I am slightly confused. I thought our current position is to disallow simultaneous legacy and modern interfaces: Please choose one endpoint to use. No need to relate all of them simultaneously!. IMHO, charm should get into 'blocked' state forcing users to remove one relation and choose legacy or modern only. @marceloneppel is it still valid position? I cannot find such blockers in charm.py code :-(

Hi @taurus-forever! In my opinion, we need to fix this in the relations to forbid them from being established simultaneously (and match Note: do NOT relate both modern and legacy interfaces simultaneously! from https://charmhub.io/postgresql/docs/e-interfaces). We should allow it only if we find a valid use case.

If we agree on that, I can create issues for that.

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

@BalabaDmintri , can you please add the test and charm code: simultaneous legacy and modern relations should not be allowed.

@BalabaDmitri BalabaDmitri force-pushed the deployment-legacy-modern-endpoints branch from 2950c65 to 0c7ab7f Compare March 20, 2024 15:44
@BalabaDmitri BalabaDmitri force-pushed the deployment-legacy-modern-endpoints branch from 7a06a24 to bab6842 Compare March 20, 2024 15:46
Copy link
Member

@marceloneppel marceloneppel left a comment

Choose a reason for hiding this comment

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

Thanks, BalabaDmintri! That's a great implementation. I left some comments to improve the code a bit.

@BalabaDmitri BalabaDmitri force-pushed the deployment-legacy-modern-endpoints branch from 4012575 to bab6842 Compare March 22, 2024 13:03
BalabaDmitri and others added 4 commits March 22, 2024 15:04
Co-authored-by: Marcelo Henrique Neppel <marcelo.neppel@gmail.com>
Co-authored-by: Marcelo Henrique Neppel <marcelo.neppel@gmail.com>
Co-authored-by: Marcelo Henrique Neppel <marcelo.neppel@gmail.com>
Co-authored-by: Marcelo Henrique Neppel <marcelo.neppel@gmail.com>
@BalabaDmitri BalabaDmitri force-pushed the deployment-legacy-modern-endpoints branch from c2c5223 to 2caa1a3 Compare March 22, 2024 14:02
@BalabaDmitri BalabaDmitri force-pushed the deployment-legacy-modern-endpoints branch from 159ea76 to 138aa3d Compare March 22, 2024 14:07
@taurus-forever
Copy link
Contributor

@BalabaDmintri can you please update PR description. It doesn't represent the content. Tnx!

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

LGTM, @dragomirp consider to merge if LGTM as well. Tnx!

@@ -0,0 +1,2 @@
# Copyright 2023 Canonical Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2024, you can ignore it here and update the next PR.

Comment on lines 49 to 51
MAILMAN3_CORE_APP_NAME,
application_name=MAILMAN3_CORE_APP_NAME,
channel="stable",
Copy link
Contributor

Choose a reason for hiding this comment

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

This charm has no updates for ages... and I would recommend to use 2 postgresql-test-app instead... but let's see how stable the current approach is.

@dragomirp
Copy link
Contributor

Hi, @BalabaDmintri, please fix the error in the unit test and sync up with main.

@BalabaDmitri BalabaDmitri force-pushed the deployment-legacy-modern-endpoints branch from a1fa5fa to c317415 Compare April 5, 2024 19:15
@BalabaDmitri BalabaDmitri force-pushed the deployment-legacy-modern-endpoints branch from 954531e to c0d39b8 Compare April 5, 2024 19:16
@BalabaDmitri BalabaDmitri reopened this Apr 10, 2024
@BalabaDmitri
Copy link
Contributor Author

@dragomirp Can you check

@BalabaDmitri BalabaDmitri changed the title Tests legacy and modern endpoints simultaneously [DPE-4106] Tests legacy and modern endpoints simultaneously Apr 17, 2024
@BalabaDmitri
Copy link
Contributor Author

@dragomirp Can you run actions

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.

4 participants