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

Move collector service account template to separate collector … #4312

Merged
merged 9 commits into from
Jan 27, 2025

Conversation

andriisoldatenko
Copy link
Contributor

@andriisoldatenko andriisoldatenko commented Jan 20, 2025

Description

This PR moves/renames extension-collector service account and cluster role, since we will use it NOT only with extensions.

resolves Issue DAQ-3329

How can this be tested?

  1. Deploy operator 1.4.0
helm upgrade dynatrace-operator oci://public.ecr.aws/dynatrace/dynatrace-operator \
  --version 1.4.0 \
  --create-namespace --namespace dynatrace \
  --install \
  --atomic
  1. Upgrade using this branch:

Assuming you have built image/or use CI one:

helm upgrade dynatrace-operator config/helm/chart/default \
                        --install \
                        --namespace dynatrace \
                        --create-namespace \
                        --atomic \
                        --set installCRD=true \
                        --set csidriver.enabled=true \
                        --set manifests=true \
                        --set image="quay.io/dynatrace/dynatrace-operator:snapshot-rename-collector-sa"
  1. Make sure everything is running; check the list of SA/ClusterRoles and bindings to make sure there are no old leftovers.

@andriisoldatenko andriisoldatenko marked this pull request as ready for review January 20, 2025 11:41
@andriisoldatenko andriisoldatenko requested a review from a team as a code owner January 20, 2025 11:41
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.18%. Comparing base (f00e14a) to head (092e79b).
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4312   +/-   ##
=======================================
  Coverage   64.18%   64.18%           
=======================================
  Files         401      401           
  Lines       26327    26327           
=======================================
  Hits        16897    16897           
  Misses       8125     8125           
  Partials     1305     1305           
Flag Coverage Δ
unittests 64.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andriisoldatenko andriisoldatenko enabled auto-merge (squash) January 20, 2025 16:18
Copy link
Contributor

@0sewa0 0sewa0 left a comment

Choose a reason for hiding this comment

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

LGTM, just few comments:

  • do we need to consider the users who use manifests directly?
  • do marketplaces handle this gracefully?

nitpick:

  • Adding a [Helm] prefix to the title is unnecessary, we even have a label for Helm 😅

@andriisoldatenko andriisoldatenko changed the title [Helm] Move collector service account template to separate collector … Move collector service account template to separate collector … Jan 21, 2025
@andriisoldatenko andriisoldatenko added the helm Changes to helm templates or values file label Jan 21, 2025
@andriisoldatenko
Copy link
Contributor Author

nitpick:
Adding a [Helm] prefix to the title is unnecessary, we even have a label for Helm 😅
@0sewa0 fixed - it was copy pasta from jira ticket description, my bad :(

@andriisoldatenko
Copy link
Contributor Author

andriisoldatenko commented Jan 21, 2025

do we need to consider the users who use manifests directly?
do marketplaces handle this gracefully?

that's a fair point, @StefanHauth. Could you please answer on it?

Copy link
Contributor

@waodim waodim left a comment

Choose a reason for hiding this comment

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

lgtm, i will approve if the 2 remaining questions are answered

@StefanHauth
Copy link
Collaborator

StefanHauth commented Jan 22, 2025

  • do we need to consider the users who use manifests directly?

This should be well covered in release notes and made very clear that some manual cleanup might be necessary.

andriisoldatenko and others added 2 commits January 27, 2025 16:23
@andriisoldatenko andriisoldatenko merged commit cef9690 into main Jan 27, 2025
14 checks passed
@andriisoldatenko andriisoldatenko deleted the rename-collector-sa branch January 27, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm Changes to helm templates or values file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants