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

[DPE-2838] Secret labels #303

Merged
merged 7 commits into from
Nov 24, 2023
Merged

[DPE-2838] Secret labels #303

merged 7 commits into from
Nov 24, 2023

Conversation

juditnovak
Copy link
Contributor

Secrets with labels (refacored to a better shape)

src/charm.py Dismissed Show dismissed Hide dismissed
@juditnovak juditnovak force-pushed the DPE-2838_secret_labels branch from 56d6bd4 to 6bef675 Compare October 25, 2023 18:58
src/charm.py Dismissed Show dismissed Hide dismissed
@juditnovak juditnovak force-pushed the DPE-2838_secret_labels branch 4 times, most recently from e795f8e to 2c55d94 Compare October 26, 2023 17:47
Comment on lines +231 to +248
value = secret.get_content().get(secret_key)
if value != SECRET_DELETED_LABEL:
return value
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do this in self.secrets.get()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this one too.

As long as hoping for unification of the <charmnam>_secrets module across charms, I'd like to leave this out.

The reason is: this is a Juju 3.1.6 > workaround.
Now we already have a bunch of charms that could drop Juju2. I'm wondering that they may be able 2 drop 3.1.5 at some point -- while other charms may still need to support it.

I'd rather not put a version-bound workaround in a lib, that has a chance for centralization.

It's a logical suggestion, but I'm wondering that --for now-- it may be better to keep it outside.
(If the moduel relamins local and OK to diverge, this could be refactored into the module)

Copy link
Member

Choose a reason for hiding this comment

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

move to lib in the future PR

src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@juditnovak juditnovak force-pushed the DPE-2838_secret_labels branch 13 times, most recently from f253472 to 778436e Compare November 3, 2023 09:56
@juditnovak juditnovak marked this pull request as ready for review November 3, 2023 13:55
@dragomirp
Copy link
Contributor

Upgrade tests seem to be failing because existing secrets on 14/edge and 14/stable are not labelled and after the upgrade the charm can no longer find them.

@juditnovak juditnovak force-pushed the DPE-2838_secret_labels branch 12 times, most recently from 48dcab3 to 11c439f Compare November 21, 2023 17:29
# We need to create a brand-new secret for this scope
else:
scope_obj = self._scope_obj(scope)
if JujuVersion.from_environ().has_secrets:
Copy link
Member

Choose a reason for hiding this comment

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

lets create such func in the lib in the future PR


return bool(self.secrets[scope].get(SECRET_CACHE_LABEL))
def _translate_field_to_secret_key(self, key: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

move to lib in the future PR

else:
scope_obj = self._scope_obj(scope)
if JujuVersion.from_environ().has_secrets:
label = generate_secret_label(self, scope)
Copy link
Member

Choose a reason for hiding this comment

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

lets generate label in the lib in the future PR

Comment on lines +231 to +248
value = secret.get_content().get(secret_key)
if value != SECRET_DELETED_LABEL:
return value
Copy link
Member

Choose a reason for hiding this comment

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

move to lib in the future PR


key = SECRET_KEY_OVERRIDES.get(key, key)
def remove_secret(self, scope: Scopes, key: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

lets move most of logic to lib in the future PR


def set_secret(self, scope: str, key: str, value: Optional[str]) -> Optional[str]:
def set_secret(self, scope: Scopes, key: str, value: Optional[str]) -> Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

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

lets move most of logic to lib in the future PR

@juditnovak juditnovak merged commit 3034b95 into main Nov 24, 2023
35 checks passed
@juditnovak juditnovak deleted the DPE-2838_secret_labels branch November 24, 2023 14:58
BON4 pushed a commit to BON4/postgresql-k8s-operator that referenced this pull request May 20, 2024
* Adding parameterized

* New data_secrets lib from data-platform-libs

* Refactoring secret functionalities

* Unittests

* Integration tests

* ops, pytest-mock libs for integration, so secrets-related fixtures would work

* Updates safe to switch from secret ID to label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants