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

support projects #428

Merged
merged 9 commits into from
Mar 11, 2023
Merged

support projects #428

merged 9 commits into from
Mar 11, 2023

Conversation

zubenkoivan
Copy link
Contributor

closes #424

@zubenkoivan zubenkoivan marked this pull request as draft March 8, 2023 09:29
@zubenkoivan zubenkoivan marked this pull request as ready for review March 9, 2023 07:52
platform_secrets/api.py Outdated Show resolved Hide resolved
platform_secrets/api.py Outdated Show resolved Hide resolved
@zubenkoivan zubenkoivan requested a review from dalazx March 9, 2023 20:31
Copy link
Contributor

@dalazx dalazx left a comment

Choose a reason for hiding this comment

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

👍

@@ -66,6 +67,10 @@ async def add_secret(self, secret: Secret) -> None:
labels = {}
if secret.org_name:
labels[SECRET_API_ORG_LABEL] = secret.org_name
if secret.project_name == secret.owner:
labels[USER_LABEL] = secret.owner.replace("/", "--")
Copy link
Contributor

@YevheniiSemendiak YevheniiSemendiak Mar 10, 2023

Choose a reason for hiding this comment

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

Unclear how we use this label later, maybe you could elaborate? (maybe, the plan was to utilize it in get_all_secrets instead of _get_owner_from_secret_name)

Copy link
Contributor

Choose a reason for hiding this comment

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

why wasn't this comment addressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, didn't notice your comment. Yes, I think the plan was to utilize it in get_all_secrets first but then I decided to refuse from this approach. This should be removed, will do.

Secret(key=key, value=value, owner=owner, org_name=org_name)
Secret(
key=key,
value=value,
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: we probably could do value=value if with_values else "" end remove if-else

Copy link
Contributor

@YevheniiSemendiak YevheniiSemendiak left a comment

Choose a reason for hiding this comment

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

Great job! 💪

@zubenkoivan zubenkoivan merged commit be23553 into master Mar 11, 2023
@zubenkoivan zubenkoivan deleted the iz/support-projects branch March 11, 2023 07:09
Comment on lines +97 to +98
if project_name:
label_selectors += [f"{PROJECT_LABEL}={project_name}"]
Copy link
Contributor

Choose a reason for hiding this comment

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

here we no longer respect USER_LABEL which we set for secrets belonging to projects named after owners.
we should have migrated to PROJECT_LABEL entirely as now these legacy and new labels cause confusion.

Copy link
Contributor Author

@zubenkoivan zubenkoivan Apr 26, 2023

Choose a reason for hiding this comment

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

I see, will remove project label selector and switch to filtering secrets in memory based on owner or project

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.

Add support of new project
3 participants