Skip to content

Conversation

marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Aug 25, 2022

Issue

  • Secrets like passwords and TLS certificates should be moved to Juju secrets in the future.
  • Currently, passwords are being set and retrieved from app relation data in multiple places, what can lead to a big refactor later (even more when we add TLS certificates to the charm).

Solution

  • Move secret implementation logic to specific functions that can be easily refactored in the future to use Juju secrets instead of app relation data.

Context

  • Unit peer relation property was also added for future use cases.
  • Some constants were created to avoid the same strings in multiple places.
  • This was already done in a very similar way in Rework secrets postgresql-k8s-operator#27.
  • Checking again Will's comment in the other PR, I saw that it makes sense for the other relation data that the charm uses (like list of endpoints). So an issue was created to move all operations related to the peer relation to another file.

Testing

  • Additional unit tests were added.
  • Tested using integration tests (to ensure the new implementation works the same way).

Release Notes

  • Add specific functions to set and get secrets.

@marceloneppel marceloneppel changed the base branch from main to new-user August 26, 2022 10:54
Base automatically changed from new-user to main August 29, 2022 16:41
@codecov-commenter
Copy link

Codecov Report

Merging #23 (f15a1b7) into main (ceeba02) will increase coverage by 0.15%.
The diff coverage is 74.54%.

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   60.11%   60.27%   +0.15%     
==========================================
  Files           6        6              
  Lines         692      730      +38     
  Branches      103      113      +10     
==========================================
+ Hits          416      440      +24     
- Misses        256      264       +8     
- Partials       20       26       +6     
Impacted Files Coverage Δ
src/charm.py 52.77% <72.54%> (+0.59%) ⬆️
src/cluster.py 54.81% <100.00%> (+0.33%) ⬆️
src/constants.py 100.00% <100.00%> (ø)

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

Copy link
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

lgtm

@marceloneppel marceloneppel merged commit c84f4a6 into main Sep 1, 2022
@marceloneppel marceloneppel deleted the rework-secrets branch September 1, 2022 17:19
BON4 pushed a commit to BON4/postgresql-operator that referenced this pull request Apr 23, 2024
* Change charm database user

* Fix unit tests

* Fix integration test call

* Fix user name in library

* Fix user

* Add default postgres user creation

* Change action name

* Rework secrets management
github-actions bot added a commit to canonical/test-runners-2-github-x64-postgresql-operator that referenced this pull request May 17, 2024
github-actions bot added a commit to canonical/test-runners-2-is-x64-postgresql-operator that referenced this pull request May 18, 2024
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