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

feat(#702): serviceaccount removal #724

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

liquidiert
Copy link
Collaborator

@liquidiert liquidiert commented Oct 22, 2024

This PR adds automatic removal of service accounts in operator terminate handler

closes #702

  - add registry url cli option for easy registry change
  - fix service annotations
  - fix replacement of kwargs with empty dict by filtering only the empty values
- feat: add service account deletion in operator terminate handler
- add registry url to up command
@liquidiert
Copy link
Collaborator Author

I merged the other PR because I wanted to test it locally and provide a custom registry url

@Schille
Copy link
Collaborator

Schille commented Oct 29, 2024

This PR addresses #702

Copy link
Contributor

@SteinRobert SteinRobert left a comment

Choose a reason for hiding this comment

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

Also a simple test whether the Image Urls are created correctly would be nice. Just instantiate the clientconfig and check whether the Image Urls are set correctly.

key, value = annotation[0].split("=")
stowaway_annotations[key] = value
except ValueError:
# handle preset values
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably display a warning or something if this happens and the parameter is ignored no?

kube_config_file=kubeconfig,
kube_context=kubecontext,
ignore_docker=True,
registry_url=kwargs.get("registry_url", ""),
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 make a dedicated kw? I think that would be cleaner and nicer for code completion in IDEs

Copy link
Collaborator Author

@liquidiert liquidiert Nov 20, 2024

Choose a reason for hiding this comment

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

Because of the construction of GefyraInstallOptions in lines 66-67. Making registry_url a dedicated keyword pops it from kwargs leading to the option not being respected anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

Still I think it would be better to make it explicit, just for code completion and typing.
You could add it to the kwargs in case a preset is set.

if not all(kwargs.values()):
kwargs = {}
# filter out empty kwargs
kwargs = {k: v for k, v in kwargs.items() if v}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more transparent to show a warning when things are filtered out no? Otherwise users might wonder.

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.

Service Account not deleted when GefyraClient is removed
3 participants