Skip to content

DPE 1083: Prevent relation extensions #50

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
Merged
merged 2 commits into from
Jan 12, 2023
Merged

DPE 1083: Prevent relation extensions #50

merged 2 commits into from
Jan 12, 2023

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented Jan 9, 2023 •
edited
Loading

Issue

  • DPE-1083
  • The charm should block in case extensions are enabled by a legacy relation

Solution

  • Changed where the charm was looking for if extensions were enabled

Context

  • Essentially the same as the fix for DPE-1034
  • The Postgresql charm should block in case relation extensions are enabled, but it was looking for them in the relation databags, not in the event data
  • Also updated action versions

Testing

  • Added integration test to verify that the charm will block

Release Notes

Fixes bug in preventing relation extensions

@codecov
Copy link

codecov bot commented Jan 9, 2023 •
edited
Loading

Codecov Report

Merging #50 (4ee6f3b) into main (362ad57) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main      #50   +/-   ##
=======================================
  Coverage   65.35%   65.35%           
=======================================
  Files           6        6           
  Lines         814      814           
  Branches      115      115           
=======================================
  Hits          532      532           
  Misses        258      258           
  Partials       24       24
Impacted Files Coverage Δ
src/relations/db.py 69.74% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dragomirp
dragomirp commented Jan 11, 2023
View reviewed changes
tests/integration/test_db.py
Comment on lines +167 to +178
await ops_test.model.deploy(
"nextcloud",
channel="edge",
application_name="nextcloud",
num_units=APPLICATION_UNITS,
)
await ops_test.model.wait_for_idle(
apps=["nextcloud"],
status="blocked",
raise_on_blocked=False,
timeout=1000,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a third party charm requesting extensions.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome finding!

@dragomirp
Copy link
Contributor Author

Codecov seems to complain that the patch doesn't change the coverage. How should we deal with such cases?

@dragomirp dragomirp marked this pull request as ready for review January 11, 2023 11:50
@taurus-forever
Copy link
Contributor

How should we deal with such cases?

@marceloneppel ? IMHO, enforcing test coverage is not something we should push now, IMHO.

taurus-forever
taurus-forever approved these changes Jan 12, 2023
View reviewed changes
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.

Tnx!

marceloneppel
marceloneppel approved these changes Jan 12, 2023
View reviewed changes
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.

It looks great! @taurus-forever, I agree about not enforcing the coverage now. And also because IMO the specific issue we see in this PR may be a bug on codecov or its action.

In this case it commented The diff coverage is 0.00%. but it passed when it commented The diff coverage is n/a. (on #43 (comment)). I know we have a minor difference between both PRs, but it seems to be a bug on codecov.

tests/integration/test_db.py
Comment on lines +167 to +178
await ops_test.model.deploy(
"nextcloud",
channel="edge",
application_name="nextcloud",
num_units=APPLICATION_UNITS,
)
await ops_test.model.wait_for_idle(
apps=["nextcloud"],
status="blocked",
raise_on_blocked=False,
timeout=1000,
)
Copy link
Member

Choose a reason for hiding this comment

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

Awesome finding!

@dragomirp dragomirp merged commit 521b03c into main Jan 12, 2023
@dragomirp dragomirp deleted the dpe-1083-preventing-relation-extensions branch January 12, 2023 13:07
BON4 pushed a commit to BON4/postgresql-operator that referenced this pull request Apr 23, 2024
@dragomirp
DPE 1083: Prevent relation extensions (canonical#50)
86fee26 
* Prevent relation extensions

* Integration test
github-actions bot added a commit to canonical/test-runners-2-github-x64-postgresql-operator that referenced this pull request May 24, 2024
@github-actions
Allure report canonical#50
f2aca5c
github-actions bot added a commit to canonical/test-runners-2-azure-arm64-postgresql-operator that referenced this pull request May 24, 2024
@github-actions
Allure report canonical#50
7e42a23
github-actions bot added a commit to canonical/test-runners-2-is-x64-postgresql-operator that referenced this pull request May 25, 2024
@github-actions
Allure report canonical#50
0e663e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taurus-forever taurus-forever taurus-forever approved these changes

@marceloneppel marceloneppel marceloneppel approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dragomirp @taurus-forever @marceloneppel