-
Notifications
You must be signed in to change notification settings - Fork 154
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 labels to use kubernetes recommended labels #4722
Conversation
💚 CLA has been signed |
This pull request does not have a backport label. Could you fix it @therealdwright? 🙏
NOTE: |
0832609
to
711f3f1
Compare
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely need a changelog entry for this change, as this affects users.
@therealdwright Thanks for your contribution and signing the CLA. |
711f3f1
to
4cc5930
Compare
I've pushed a change log entry as requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changelog.
Head branch was pushed to by a user without write access
4cc5930
to
8f6c679
Compare
@blakerouse I have rebased on main as the |
@therealdwright Yes it requires an authorized user to approve and run the workflows. This ensures we can review the change before we allow it to run on an actual machine. |
Oops, sorry about that, @therealdwright! Here are the logs that are relevant:
To fix that, I ran |
buildkite test this |
|
04de6e6
to
c96ec46
Compare
Head branch was pushed to by a user without write access
c96ec46
to
114baf8
Compare
This pull request is now in conflicts. Could you fix it? 🙏
|
buildkite test it |
@gizas Unfortunately this PR hasn't been looked at for quite a while, would you mind reviewing it again in light of all the recent changes that could have affected the kustomize templates? |
@@ -66,6 +66,8 @@ spec: | |||
valueFrom: | |||
fieldRef: | |||
fieldPath: metadata.name | |||
- name: STATE_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gizas this isn't a label change, so is this change required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, not needed
@@ -66,6 +66,8 @@ spec: | |||
valueFrom: | |||
fieldRef: | |||
fieldPath: metadata.name | |||
- name: STATE_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gizas this isn't a label change, so is this change required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not me that added this but I will rebase and remove.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, minus @pkoutsovasilis comment about the STATE_PATH variable.
Kubernetes have not used the `app` or `k8s-app` for some time and have provided guidance on what the recommended labels are for some time as seen in their [docs](https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/). This commit updates all manifests/docs to use this approach.
deb2225
to
a39df6d
Compare
Sonar and CI check will never run as there was no code change hence force merging this as both @swiatekm and @blakerouse approved. |
Thanks @therealdwright for pushing this through! |
Kubernetes have not used the `app` or `k8s-app` for some time and have provided guidance on what the recommended labels are for some time as seen in their [docs](https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/). This commit updates all manifests/docs to use this approach. Co-authored-by: Julien Lind <julien.lind@elastic.co> (cherry picked from commit f2af90f)
Kubernetes have not used the `app` or `k8s-app` for some time and have provided guidance on what the recommended labels are for some time as seen in their [docs](https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/). This commit updates all manifests/docs to use this approach. Co-authored-by: Julien Lind <julien.lind@elastic.co> (cherry picked from commit f2af90f) Co-authored-by: Daniel Wright <danielwright@bitgo.com>
What does this PR do?
Kubernetes have not used the
app
ork8s-app
for some timeand have provided guidance on what the recommended labels are
for some time as seen in their docs.
This commit updates all manifests/docs to use this approach.
Why is it important?
Users who choose to adopt kubernetes label best practises will not have to fork elastic manifests and can instead rely on the upstream ones directly.
Checklist
./changelog/fragments
using the changelog toolDisruptive User Impact
Users will need to upgrade using
kubectl replace
as some of the labels are immutable