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

Update resources if already exist #9

Merged
merged 1 commit into from
May 31, 2024
Merged

Update resources if already exist #9

merged 1 commit into from
May 31, 2024

Conversation

ArthurSens
Copy link
Member

While trying out poctl, I'm constantly being forced to delete resources before trying again.

I think it's a better experience if we use a CreateOrUpdate strategy here 😬

What do you think?

Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
@ArthurSens ArthurSens requested a review from nicolastakashi May 29, 2024 00:06
if err != nil {
logger.With("error", err.Error()).Error("error while creating ServiceAccount")
logger.With("error", err.Error()).Error("error while creating ServiceAccount", "serviceAccount", fmt.Sprintf("%s/%s", namespace, manifests.ServiceAccount.GetName()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest using the With api from slog so we can have it as a log attribute.

logger.With("error", err.Error()).With("serviceAccount", manifests.ServiceAccount.GetName()).Error("error while creating ServiceAccount")

Copy link
Member Author

@ArthurSens ArthurSens May 29, 2024

Choose a reason for hiding this comment

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

I'm not sure if that's the indented usage for With() 🤔 (slog's With docs)

With() creates a new logger object with the arguments passed as default keys. That's a common pattern used in several logging libraries. An example is how we use logger.With() in prometheus-operator:

  • Here we add a default key to the prometheus-controller, so all logs from this controller have component=prometheuscontroller.
  • The same thing is done with all other controllers[1][2][3]

Further reading slog's documentation, I see that adding arguments to methods such as log.Error() or log.Info() will add those arguments as key=values in the logs. This is the behavior I see when running poctl create stack as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, lgtm

@nicolastakashi
Copy link
Collaborator

I left a minor comment, but general looks good to me.
We can think about using dynamic client later to reduce the amount of methods, since we might and with one createOrUpdateMethod for each resource type.

@ArthurSens ArthurSens merged commit 0ea198d into main May 31, 2024
4 checks passed
@ArthurSens ArthurSens deleted the create-or-update branch May 31, 2024 13:34
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.

2 participants