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.

Sign up for GitHub

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

Jump to bottom

Only deploy ConfigMaps and Secrets if needed #958

Merged
merged 3 commits into from
Mar 23, 2023
Merged

Only deploy ConfigMaps and Secrets if needed #958

merged 3 commits into from
Mar 23, 2023

Conversation

Starkteetje
Copy link
Member

@Starkteetje Starkteetje commented Mar 20, 2023 •
edited

Only deploy alerting ConfigMap and Secret if defined

Description

Previously, we deployed some resources even if they weren't used in the deployment. This reduces the number of instances where that happens

Checklist

  • PR is rebased to/aimed at branch develop
  • PR follows Contributing Guide
  • Added tests (if necessary)
  • Extended README/Documentation (if necessary)
  • Adjusted versions of image and Helm chart in values.yaml and Chart.yaml (if necessary)

@github-advanced-security
Copy link

You have successfully added a new Scorecard configuration supply-chain/online-scm. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@github-advanced-security
Copy link

You have successfully added a new Scorecard configuration supply-chain/local. As part of the setup process, we have scanned this repository and found 45 existing alerts. Please check the repository Security tab to see all alerts.

@Starkteetje Starkteetje changed the title Only deploy alerting ConfigMap and Secret if defined Only deploy ConfigMaps and Secrets if needed Mar 20, 2023
@Starkteetje Starkteetje force-pushed the alerting-resources-if-alerting branch 2 times, most recently from 492ed6e to 2053d0e Compare March 20, 2023 15:35
@Starkteetje Starkteetje force-pushed the alerting-resources-if-alerting branch from 2053d0e to 2463b07 Compare March 20, 2023 16:22
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.15 ⚠️

Comparison is base (c63e173) 97.13% compared to head (2463b07) 96.98%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #958      +/-   ##
===========================================
- Coverage    97.13%   96.98%   -0.15%     
===========================================
  Files           23       23              
  Lines         1290     1293       +3     
===========================================
+ Hits          1253     1254       +1     
- Misses          37       39       +2
Impacted Files Coverage Δ
connaisseur/config.py 95.61% <60.00%> (-1.69%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Starkteetje Starkteetje force-pushed the alerting-resources-if-alerting branch 2 times, most recently from d591615 to 8baf759 Compare March 20, 2023 16:55
@Starkteetje Starkteetje marked this pull request as ready for review March 20, 2023 16:56
xopham
xopham previously requested changes Mar 21, 2023
View reviewed changes
Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

In order to not have unrelated changes in other files and avoid merge conflicts, I'd prefer to keep PRs focused on main goal

tests/integration/integration-test.sh Outdated Show resolved Hide resolved
tests/integration/integration-test.sh Outdated Show resolved Hide resolved
tests/integration/integration-test.sh Outdated Show resolved Hide resolved
tests/integration/integration-test.sh Outdated Show resolved Hide resolved
@Starkteetje Starkteetje force-pushed the alerting-resources-if-alerting branch from 8baf759 to bc003d6 Compare March 21, 2023 13:58
phbelitz
phbelitz previously approved these changes Mar 23, 2023
View reviewed changes
helm/templates/_helpers.tpl Outdated Show resolved Hide resolved
@Starkteetje
ci: Fix upgrade test
dbeb632 
Before this commit, the upgrade test did use the previous Connaisseur image, so in fact did only upgrade the Helm part, but not the actual application. This commit fixes it to use the upgraded Chart and the upgraded application
@Starkteetje
fix: Only deploy alerting ConfigMap and Secret if defined
83cb3d0
@Starkteetje
fix: Only deploy config Secrets if defined
fa1bb60
@Starkteetje Starkteetje force-pushed the alerting-resources-if-alerting branch from bc003d6 to fa1bb60 Compare March 23, 2023 17:05
@Starkteetje Starkteetje requested a review from xopham March 23, 2023 17:20
@Starkteetje Starkteetje merged commit fa1bb60 into develop Mar 23, 2023
@Starkteetje Starkteetje deleted the alerting-resources-if-alerting branch March 23, 2023 17:21
@Starkteetje Starkteetje mentioned this pull request Mar 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phbelitz phbelitz phbelitz approved these changes

@xopham xopham Awaiting requested review from xopham

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Starkteetje @codecov-commenter @xopham @phbelitz