Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Basic Prometheus support #28

Merged
merged 19 commits into from Feb 24, 2023
Merged

Basic Prometheus support #28

merged 19 commits into from Feb 24, 2023

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Feb 20, 2023 •
edited

This patch adds very basic prometheus support to the agent and server. It also has a fix in it so that changes to the agent or server configmaps reload those pods.

Partially implements: #27

marcofranssen
marcofranssen suggested changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

Couple of suggestion and questions. Also ensure to generate the helm-docs and bump the chart version as stated in the CONTRIBUTING.md to fix the CI.

charts/spire/charts/spire-agent/templates/daemonset.yaml Show resolved Hide resolved
charts/spire/charts/spire-server/templates/configmap.yaml Outdated Show resolved Hide resolved
charts/spire/charts/spire-server/values.yaml Show resolved Hide resolved
charts/spire/values.yaml Outdated Show resolved Hide resolved
charts/spire/values.yaml Outdated Show resolved Hide resolved
charts/spire/charts/spire-agent/templates/configmap.yaml Outdated Show resolved Hide resolved
@kfox1111
Copy link
Contributor Author

Couple of suggestion and questions. Also ensure to generate the helm-docs and bump the chart version as stated in the CONTRIBUTING.md to fix the CI.

This is a bit problematic until #30 is resolved.

There already is a 0.1.1 pr in the works: https://github.com/spiffe/helm-charts/pull/23/files

And didn't want to be constantly rebasing patches based on the version number conflict as patches get in. Was hoping for a review and work out most of the issues before actually putting a version number on it.

marcofranssen
marcofranssen previously approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@marcofranssen marcofranssen left a comment •
edited

Choose a reason for hiding this comment

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

One more question for my understanding, didn't test the PR here on local. I see DCO is not on all commits. For the rest LGTM 🚀

@kfox1111
Copy link
Contributor Author

I guess I missed one. :/ I can fix it.

Whats the bigger plan anyway? Are we planning on flattening before merge, in which case its not so much an issue or do we maintain the whole history of changes to the PR?

@marcofranssen
Copy link
Contributor

marcofranssen commented Feb 21, 2023 •
edited

For this small one we can probably squash. For larger ones I usually prefer to keep history, so I can document specific changes in commits. If it is a PR with a single commit I usually do a rebase merge which gives clean history.

git rebase main --signoff
git push --force-with-lease

can resolve it for this PR. But feel free to squash this one.

@kfox1111
Copy link
Contributor Author

Part of the problem seems to be this:

commit 9dee09e38e25fd4e2aeb7d49b9b50d7b09181315 (tag: spire-0.1.0)
Merge: 258a5a2 cb3f940
Author: Marco Franssen <marco.franssen@gmail.com>
Date:   Mon Feb 20 10:59:32 2023 +0100

    Merge pull request #14 from spiffe/base-chart

No signoff.

I'll try the rebase though.

@kfox1111
Copy link
Contributor Author

k. rebased.

@marcofranssen
Copy link
Contributor

Could you update the docs ./helm-docs.sh.

@marcofranssen marcofranssen dismissed their stale review February 23, 2023 09:30

Triggered by Faisals question we might miss small thing.

@kfox1111
Copy link
Contributor Author

Merge conflict resolved.

@marcofranssen
Copy link
Contributor

Since we now have the test setup in place, should we include a test for the Prometheus values?

@kfox1111
Copy link
Contributor Author

Rebased so tests would exist, then added one.

@marcofranssen marcofranssen force-pushed the prometheus branch 2 times, most recently from e6ef725 to d3062a7 Compare February 24, 2023 20:19
kfox1111 and others added 19 commits February 24, 2023 22:06
@kfox1111 @marcofranssen
Basic Prometheus support
a276c98 
This patch adds very basic prometheus support to the agent and server
It also has a fix in it so that changes to the agent or server
configmaps reload those pods.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Fix spacing.
6f2daf0 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Update default values
871b3cd 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Update readmes
b375d53 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Make ports configurable and fix docs
c0d16f4 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Add test
8eac7a1 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Fix docs and bump version for now.
9dc5f8f 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Fix lint and docs.
25f7d27 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Add oidc-discovery exporter and specify ports.
c2cbef4 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Remove merge conflict issue
8dcf3d7 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Add a comment
26e57dd 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Enable prom on the controller manager
df4e1d3 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Basic Prometheus support
b3a24c7 
This patch adds very basic prometheus support to the agent and server
It also has a fix in it so that changes to the agent or server
configmaps reload those pods.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Update default values
07847b5 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Update readmes
8590073 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Remove merge conflict issue
1a0c5ba 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@marcofranssen
Fixup adding telemetry port in wrong place
e5623e4 
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
@marcofranssen
Revert version bump
c44c48a 
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
@kfox1111 @marcofranssen
Ensure metrics config is always there for controller manager
0839037 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
marcofranssen
marcofranssen approved these changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@kfox1111
Copy link
Contributor Author

lgtm too.

@marcofranssen marcofranssen merged commit 35eb3bb into main Feb 24, 2023
@marcofranssen marcofranssen deleted the prometheus branch February 24, 2023 21:39
@marcofranssen marcofranssen added this to the Initial release milestone Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@faisal-memon faisal-memon faisal-memon left review comments

@marcofranssen marcofranssen marcofranssen approved these changes

@developer-guy developer-guy Awaiting requested review from developer-guy

@dennisgove dennisgove Awaiting requested review from dennisgove

@dfeldman dfeldman Awaiting requested review from dfeldman

@mrsabath mrsabath Awaiting requested review from mrsabath

Assignees

@marcofranssen marcofranssen

Labels
None yet
Projects
None yet
Milestone
0.6.x
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kfox1111 @marcofranssen @faisal-memon